Implement sqlx::IntoArguments for Values #275

nitnelave · 2022-03-22T13:40:12Z

Fixes #273

PR Info

Closes Implement sqlx::IntoArguments for Values #273

Adds

Implements sqlx::IntoArguments for Values.
Implement IntoIterator for Values.

Breaking Changes

It will break for users who have enabled an optional feature (e.g. with-chrono) and an sqlx backend.
To avoid that, we could add yet another feature to guard the conversion, but it would make the API less discoverable. On the other hand, the errors you get in the scenario above are very clear (it tells you which feature you need to enable).

nitnelave · 2022-03-22T16:46:47Z

Small question: when writing a u64 to Postgres or Sqlite, since they don't support that format, I try to convert it to a i64 and unwrap if it fails. Would it perhaps be better to fail-fast by panicking for all inputs, forcing the user to do the conversion themselves?
Similarly, for Postgres, I promote small unsigned to bigger signed integers (because PG doesn't support unsigned). That will always work, but might not be ideal? Not sure about that one, if we end up serializing that to a string it shouldn't matter. It would be a bit annoying on the user side to be forced to convert everything to signed just because, even though it works like that.

nitnelave · 2022-03-23T11:25:34Z

Example usage in a real project: lldap/lldap#127

Sytten · 2022-03-23T11:57:28Z

This is a big shift from the macro (https://github.com/SeaQL/sea-query/blob/master/src/driver/sqlx_sqlite.rs) that was used previously to generate that.

Like I suggested a while ago I think those integrations should be in different crates then you can greatly simplify the number of features and stay consistent between this and the bind macro.

Different crates would also allow me to write the diesel integration.

nitnelave · 2022-03-24T08:18:12Z

One problem with a different crate is that you can't condition on the sea query features that are enabled (or can you? I didn't find out how to do that for sqlx here). That means that you'll have to repeat the same feature flags for the other crate.

Here we can remove all the feature flags that I added with their sqlx versions, you'll just get a not very clear error message if you enable a feature for sea query but not sqlx.

How would the conversion look like in the other crate? Adding a .to_sqlx method to Values, via a trait?

Anyway, we should at least keep the IntoIterator part of this PR, it can even be used in the existing macros to avoid some copies.

Sytten · 2022-03-24T13:55:41Z

If we do a separate crate I think its fair to have duplicate features and that a feature X can also enable it on sea-query (like you already did) so you dont have to specify it twice.

I mean yeah that could work too. I am just trying to think on what is more confusing for the user. I think that if you enable sqlx-mysql and with-chrono on sea-query its fair to assume that you need to enable chrono on the sqlx dependency too. Documentation can help on that.

Hum yeah this is more annoying and one major point in favor of not splitting in another crate. Maybe then its a matter of splitting it in modules so they are not all in one giant file and its easy to enable/disable each flavour. I do plan on bringing my diesel implementation in the same format so lets try to make that work.

tyt2y3 · 2022-03-24T14:32:23Z

Welcome back, long time no see!

Anyway, we should at least keep the IntoIterator part of this PR, it can even be used in the existing macros to avoid some copies.

Agree.

The reason I don't want to include sqlx as a dependency is because yeah, it forces us to select a runtime and that's not where sea-query should do. Ideally, sqlx (like postgres) would split out a types crate so that we can only depend on the pure type definitions.

I also can't think of a better idea than moving this IntoArguments feature to another sub-crate.

nitnelave · 2022-03-24T15:33:29Z

The reason I don't want to include sqlx as a dependency is because yeah, it forces us to select a runtime

Only for tests. Otherwise, the only requirement is that in the final binary, a runtime is selected. It's still up to the user to choose what runtime they want. If we have a non-default feature like I added there, it's not a problem (and I only added it to make --all-features work, otherwise it's really not needed).

Yeah, ideally sqlx would split out the core types, but that's non-trivial work.

Moving IntoArguments into a sub-crate makes the interface less friendly, in my opinion.

nitnelave · 2022-03-24T23:56:01Z

Another advantage of IntoArguments vs the macros is that it supports dynamic DB backend, the Any type of sqlx. It delays the conversion until sqlx calls it, at which point they know what backend is actually used.

nitnelave · 2022-04-03T22:11:38Z

Hmm, no, actually this won't help with the argument resolution, we'd still have to implement it for Any as well.

Anyway, what's the decision on this? Do we want to cut this back to only IntoIterator, or keep it like this?

Sytten · 2022-04-03T22:59:45Z

I think this needs to wait on the merge of #292

ikrivosheev · 2022-04-04T10:51:14Z

@Sytten I agree. @nitnelave If you need help adapting your PR, I'm ready to help.

nitnelave · 2022-04-04T10:53:02Z

Thanks for the offer @ikrivosheev ! I should be able to handle it myself, once I understand what I'm adapting to :) What do we want to implement? Are we still on board for the intoargument trait?

nitnelave · 2022-04-07T09:24:14Z

I see that #292 is merged. AFAICT, it's just a refactoring of the drivers, so I'm not sure it changes much our approach here.
What do we want to do for this? Do we want to implement IntoArguments? I'd love to have IntoArguments for Any as well.

Sytten · 2022-04-14T03:33:20Z

With rust 1.60 we can now do crate?/feature which is ideal for the use case we have here. Though requiring 1.60 would be a bit brutal. @tyt2y3 @billy1624

nitnelave · 2022-04-18T10:05:05Z

@tyt2y3 any opinion here? Do we want to proceed with this PR or not? If we do, I'll add support for the Any database as well.

ikrivosheev · 2022-04-18T10:52:54Z

@nitnelave I like idea @tyt2y3 about: don't bring sqlx into sea-query. Two variants:

move into other subcrate
move into drivers

But I don't understand what is different between this and current driver implants?

nitnelave · 2022-04-18T11:05:48Z

The problem is that we can't implement a trait on Values in another crate. The only possibility would be that we somehow extract the entire Values struct (and dependencies) to another crate, and implement the trait there, but that's still ugly (sea-query depends on that type definition, which depends on sqlx for the trait).

Note that the sqlx dependency is there only if you enable the sqlx feature.

The reason why I consider this approach superior to drivers is:

The syntax is nicer, it's just "normal" sqlx syntax, with no macro involved.
It works with Sqlx's expected types and traits directly, instead of working around them with multiple calls to bind
It works with generics: if the function called works on a Pool<DB>, then implementing the trait will make sure that the correct DB is picked, whereas with macros you have to specify directly which DB you're using.

nitnelave · 2022-04-18T11:28:31Z

One idea I can come up with is to have a sub crate that implements a new trait on the various queries, adding a .build_sqlx method instead of build, that returns a wrapper type around Values with IntoArguments implemented.

It's kinda hard to discover though, in that case we should probably add some comments here around the sqlx feature referencing that crate.

Sytten · 2022-04-18T17:56:38Z

I would be in favour of moving values to the driver crate, maybe it could be renamed to better express that.

nitnelave · 2022-04-18T20:38:06Z

That doesn't really solve the issue of depending on sqlx: sea-query depends on the driver crate, which depends on sqlx. Plus you'll have to move all the dependencies of Value to that crate as well. I don't think it would bring much clarity.

ikrivosheev · 2022-04-18T21:21:43Z

@nitnelave, no, sea-query-driver doesn't depend on sqlx...

ikrivosheev · 2022-04-18T21:25:58Z

But I like your idea and the interface is convenient.

Sytten · 2022-04-18T21:31:57Z

Agreed but it is a solved problem on 1.60 though as you can mix optional dependencies with features so its just a matter of waiting a couple of weeks for people to upgrade IMO.

nitnelave · 2022-04-18T22:01:35Z

@nitnelave, no, sea-query-driver doesn't depend on sqlx...

Well, not now, but if we implement IntoArguments for Values in sea-query-driver, then it will (to the same extent that we were talking about sea-query depending on sqlx). The dependency doesn't really change, it's just hidden in a subcrate that we depend on.

billy1624 · 2022-04-21T08:55:47Z

We could implement sqlx::IntoArguments inside sea-query-driver with macros, which means sea-query-driver doesn't depends on sqlx. The macros will define its own Values which wrap the sea_query::Values.

pub struct Values(pub sea_query::Values);

Then, IntoIterator and sqlx::IntoArguments can be implemented as follows

impl IntoIterator for Values {
    type Item = sea_query::Value;
    type IntoIter = std::vec::IntoIter<Self::Item>;

    fn into_iter(self) -> Self::IntoIter {
        self.0.into_iter()
    }
}

impl<'q> sqlx::IntoArguments<'q, sqlx::postgres::Postgres> for Values {
    ...
}

nitnelave · 2022-04-21T10:01:08Z

@billy1624 but you still need the macros to wrap the values. Can we merge this with the idea I had above, implementing a new method (via a trait) on the queries that pre-wraps the values?

tyt2y3 · 2022-04-21T16:02:05Z

@billy1624 but you still need the macros to wrap the values. Can we merge this with the idea I had above, implementing a new method (via a trait) on the queries that pre-wraps the values?

Oh, so it's like the current PR, but in an external crate?

We could implement sqlx::IntoArguments inside sea-query-driver with macros, which means sea-query-driver doesn't depends on sqlx. The macros will define its own Values which wrap the sea_query::Values.

I guess this is the same as what we are doing with Rusqlite now.

nitnelave · 2022-04-21T17:42:11Z

@tyt2y3 it would be a mix of my external crate proposal and @billy1624 's proposal: the driver macros introduce a new wrapping Values type and implement IntoArgument for it. And in addition, they create a new trait and implement it for all queries, that adds a .build-sqlx method which calls .build and wraps the returned Values in the new type. That way from outside you just have to use the new trait, call build_sqlx and everything works fine.

tyt2y3 · 2022-04-22T00:27:56Z

build_sqlx and everything works fine

Sounds like an intricate plan. But that's our end goal yeah. As long as 1) it does not affect existing users 2) is opt-in we will merge it and see how it works in action, and might actually replace our current driving mechanism (which is a bit hacky IMO)

nitnelave · 2022-04-25T14:25:43Z

I updated the implementation, it should be rather complete now. I also added an example for using with any, the flow is pretty nice (but the data limitations are pretty tough, you're pretty much limited to the very basic types).

@Sytten I'm not sure the two drivers will have anything in common: in the case of sqlx we need to implement IntoArguments which is sqlx-specific, and same goes for diesel I expect. We could fit all under a same type, but I'm not sure why we would do that.

At most I can picture a shared wrapper, something like BindValues but the rest is very implementation-dependent.

nitnelave · 2022-04-25T14:36:03Z

One difference between this and the driver approach (more of a difference of choice): the drivers "support" the time arguments for sqlite by calling time_as_naive_utc_in_string. But that's going to fail on the way back, decoding the time argument. If the support is not built into sqlx, I'm of the opinion that it's up to the user to make a well-behaved wrapper or find an alternative.

EDIT: in the example, it's asymmetric: there is custom code to decode, but not to encode. With my update, it makes it more consistent by making the encoding appear.

Sytten · 2022-04-25T15:45:17Z

Agreed that is what I was suggesting, a BindValues and modifying the select trait implementation so its always the same function name to convert to this type.

nitnelave · 2022-04-25T15:46:43Z

Any suggestion for the function name? build_to_bind? build_bind_values?

Sytten · 2022-04-25T16:01:09Z

Hum no particular idea, just throwing a couple more in the hat: build_binding, build_bind, build_binder.
Heck even just bind could be good maybe?

nitnelave · 2022-04-25T16:05:11Z

Hmm, I disagree with bind, it doesn't bind at all, it just builds. The values produced can be bound easily, but this doesn't bind.

Sytten · 2022-04-25T16:12:40Z

Them a build_bindable returning a BindableValues could be more accurate, but I really dont have a strong opinion on it

nitnelave · 2022-04-29T07:13:33Z

@billy1624 @tyt2y3 any opinion on the Values/BindValues/build_bindable issue? Or the PR as a whole?

billy1624

Thanks! @nitnelave

billy1624 · 2022-04-29T11:37:59Z

sea-query-binders/src/sqlx_any.rs

+                    args.add(i.map(Into::<i64>::into));
+                }
+                Value::BigUnsigned(i) => {
+                    args.add(i.map(|i| <i64 as std::convert::TryFrom<u64>>::try_from(i).unwrap()));


This might result in unexpected panic
I guess conversion is required because PostgreSQL doesn't support u64?

Yes. How else would you handle this? Any is limited to the subset that every DB supports.

billy1624 · 2022-04-29T11:42:10Z

sea-query-binders/src/sqlx_mysql.rs

+                Value::ChronoDateTimeWithTimeZone(_) => {
+                    panic!("MySql doesn't support fixed offset chrono types");
+                }


We can bind it as string just like what we did at

sea-query/sea-query-driver/src/sqlx_mysql.rs

Line 24 in 719fce6

v @ Value::ChronoDateTimeWithTimeZone(_) => query.bind(v.chrono_as_naive_utc_in_string()),

billy1624 · 2022-04-29T11:48:41Z

sea-query-binders/src/sqlx_postgres.rs

+                Value::BigUnsigned(i) => {
+                    args.add(i.map(|i| <i64 as std::convert::TryFrom<u64>>::try_from(i).unwrap()));
+                }


This might result in unexpected panic

Same here.

Btw... I think the related implementation inside sea-query-driver is faulty as well. We just case u64 as i64. It will (integer) underflow silently.

sea-query/sea-query-driver/src/sqlx_postgres.rs

Line 112 in 719fce6

Value::BigUnsigned(v) => bind!(v, i64),

CC @tyt2y3 @ikrivosheev

There is no correct behavior here, unless you just want to panic on every u64 (and same for Any).

WDYT?

sea-query-binders/src/sqlx_sqlite.rs

examples/sqlx_any/Cargo.toml

ikrivosheev

@nitnelave thank you for contribution! Some comments

sea-query-binders/Cargo.toml

nitnelave · 2022-05-11T13:10:47Z

@ikrivosheev @billy1624 Any other comments?

ikrivosheev

@nitnelave, thank you! Sory for delay. LGTM!

nitnelave · 2022-05-23T07:07:43Z

Thanks for the review! Can we merge this, then?

sea-query-binders/Cargo.toml

Fixes SeaQL#273

sea-query-binders/src/sqlx_postgres.rs

nitnelave · 2022-05-31T15:16:23Z

@tyt2y3 when you have time for a review :)

nitnelave · 2022-06-15T08:57:32Z

Any chance of a review?

tyt2y3 · 2022-06-26T06:03:26Z

I am about to make some changes and merge into master

nitnelave mentioned this pull request Mar 25, 2022

usage of umlauts results in corrupted user lldap/lldap#123

Closed

nitnelave mentioned this pull request Apr 26, 2022

Support for MySQL/PostgreSQL lldap/lldap#87

Closed

billy1624 reviewed Apr 29, 2022

View reviewed changes

ikrivosheev reviewed Apr 29, 2022

View reviewed changes

sea-query-binders/Cargo.toml Show resolved Hide resolved

ikrivosheev approved these changes May 16, 2022

View reviewed changes

billy1624 reviewed May 23, 2022

View reviewed changes

sea-query-binders/Cargo.toml Outdated Show resolved Hide resolved

billy1624 requested a review from tyt2y3 May 23, 2022 07:24

nitnelave force-pushed the sqlx_intoarguments branch from 85db1ab to 7d27267 Compare May 23, 2022 08:57

Introduce a binders crate

445b660

Fixes SeaQL#273

nitnelave force-pushed the sqlx_intoarguments branch from 7d27267 to 445b660 Compare May 23, 2022 09:08

ikrivosheev reviewed May 23, 2022

View reviewed changes

sea-query-binders/src/sqlx_postgres.rs Show resolved Hide resolved

nitnelave mentioned this pull request May 23, 2022

Query binders: Add support for postgres arrays #336

Closed

tyt2y3 changed the base branch from master to sea-query-binder June 26, 2022 06:02

tyt2y3 merged commit 938fe29 into SeaQL:sea-query-binder Jun 26, 2022

tyt2y3 mentioned this pull request Jun 26, 2022

Sea query binder #361

Merged

nitnelave mentioned this pull request Jun 26, 2022

Introduce dynamic query/schema builder factory method #362

Closed

nitnelave deleted the sqlx_intoarguments branch July 14, 2022 15:46

tkporter mentioned this pull request Jan 4, 2024

postgres: silently uses signed integers when unsigned integers are specified in schema #731

Open

Implement sqlx::IntoArguments for Values #275

Implement sqlx::IntoArguments for Values #275

Conversation

nitnelave commented Mar 22, 2022

PR Info

Adds

Breaking Changes

nitnelave commented Mar 22, 2022

nitnelave commented Mar 23, 2022

Sytten commented Mar 23, 2022

nitnelave commented Mar 24, 2022

Sytten commented Mar 24, 2022

tyt2y3 commented Mar 24, 2022 • edited Loading

nitnelave commented Mar 24, 2022 • edited Loading

nitnelave commented Mar 24, 2022

nitnelave commented Apr 3, 2022

Sytten commented Apr 3, 2022

ikrivosheev commented Apr 4, 2022

nitnelave commented Apr 4, 2022

nitnelave commented Apr 7, 2022

Sytten commented Apr 14, 2022

nitnelave commented Apr 18, 2022

ikrivosheev commented Apr 18, 2022

nitnelave commented Apr 18, 2022

nitnelave commented Apr 18, 2022

Sytten commented Apr 18, 2022

nitnelave commented Apr 18, 2022

ikrivosheev commented Apr 18, 2022

ikrivosheev commented Apr 18, 2022

Sytten commented Apr 18, 2022

nitnelave commented Apr 18, 2022

billy1624 commented Apr 21, 2022

nitnelave commented Apr 21, 2022

tyt2y3 commented Apr 21, 2022 • edited Loading

nitnelave commented Apr 21, 2022

tyt2y3 commented Apr 22, 2022

nitnelave commented Apr 25, 2022

nitnelave commented Apr 25, 2022 • edited Loading

Sytten commented Apr 25, 2022

nitnelave commented Apr 25, 2022

Sytten commented Apr 25, 2022

nitnelave commented Apr 25, 2022

Sytten commented Apr 25, 2022 • edited Loading

nitnelave commented Apr 29, 2022

billy1624 left a comment

Choose a reason for hiding this comment

billy1624 Apr 29, 2022

Choose a reason for hiding this comment

nitnelave Apr 29, 2022

Choose a reason for hiding this comment

billy1624 Apr 29, 2022

Choose a reason for hiding this comment

nitnelave Apr 29, 2022

Choose a reason for hiding this comment

billy1624 Apr 29, 2022

Choose a reason for hiding this comment

nitnelave Apr 29, 2022

Choose a reason for hiding this comment

ikrivosheev left a comment

Choose a reason for hiding this comment

nitnelave commented May 11, 2022

ikrivosheev left a comment

Choose a reason for hiding this comment

nitnelave commented May 23, 2022

nitnelave commented May 31, 2022

nitnelave commented Jun 15, 2022

tyt2y3 commented Jun 26, 2022

tyt2y3 commented Mar 24, 2022 •

edited

Loading

nitnelave commented Mar 24, 2022 •

edited

Loading

tyt2y3 commented Apr 21, 2022 •

edited

Loading

nitnelave commented Apr 25, 2022 •

edited

Loading

Sytten commented Apr 25, 2022 •

edited

Loading