New serialization framework can block delegating to existing implementations

[Ten0]

As of #2931, we often can't delegate ToSql implementations to existing implementations anymore, e.g.:

struct SerializeAsi64 {
    val: u64,
}
impl ToSql<BigInt, Pg> for SerializeAsi64 {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Pg>) -> Result {
        let v: i64 = self.val.try_into()?;
        <i64 as ToSql<BigInt, Pg>>::to_sql(&v, out)
    }
}

error[E0597]: `v` does not live long enough
  --> src/lib.rs:29:44
   |
27 |     fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Pg>) -> Result {
   |               -- lifetime `'b` defined here
28 |         let v: i64 = self.val.try_into()?;
29 |         <i64 as ToSql<BigInt, Pg>>::to_sql(&v, out)
   |         -----------------------------------^^------
   |         |                                  |
   |         |                                  borrowed value does not live long enough
   |         argument requires that `v` is borrowed for `'b`
30 |     }
   |     - `v` dropped here while still borrowed

(playground)

I'm wondering whether we'd like to do something to allow it, like this:

pub trait ToSql<'r, 'b, A, DB> {
    /// See the trait documentation.
    fn to_sql(&'r self, out: &mut Output<'b, '_, DB>) -> Result;
}

impl<'r, 'b, 'a: 'b, A, T, DB> ToSql<'r, 'b, A, DB> for &'a T
where
    T: ToSql<'a, 'b, A, DB> + ?Sized,
{
    fn to_sql(&'r self, out: &mut Output<'b, '_, DB>) -> Result {
        (*self).to_sql(out)
    }
}

impl<'r, 'b> ToSql<'r, 'b, BigInt, Pg> for i64 {
    fn to_sql(&'r self, out: &mut Output<'b, '_, Pg>) -> Result {
        out.write_val(*self)
    }
}
impl<'b, 'r: 'b> ToSql<'r, 'b, BigInt, Pg> for str {
    fn to_sql(&'r self, out: &mut Output<'b, '_, Pg>) -> Result {
        out.write(self)
    }
}

(playground)

or like this:

pub trait ToSqlStatic<A, DB> {
    fn to_sql(&self, out: &mut Output<'_, '_, DB>) -> Result;
}
impl<T, A, DB> ToSql<A, DB> for T
where
    T: ToSqlStatic<A, DB>,
{
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, DB>) -> Result {
        <T as ToSqlStatic<A, DB>>::to_sql(self, out)
    }
}

(playground)

Workaround

My current workaround is to copy-paste from the Diesel implementation:
EDIT: If backend's BindCollector is RawBytesBindCollector, using reborrow works:

impl<DB> diesel::serialize::ToSql<diesel::sql_types::Int8, DB> for CustomType
where
	DB: diesel::backend::Backend<BindCollector = diesel::query_builder::bind_collector::RawBytesBindCollector<DB>>,
	i64: diesel::serialize::ToSql<diesel::sql_types::Int8, DB>,
{
	fn to_sql<'b>(&'b self, out: &mut diesel::serialize::Output<'b, '_, DB>) -> diesel::serialize::Result {
		let n: i64 = self.inner.get().try_into()?;
		diesel::serialize::ToSql::<diesel::sql_types::Int8, DB>::to_sql(&n, &mut out.reborrow())
	}
}

[weiznich]

You can use Output::reborrow to shorten the output lifetime for the inner to_sql call.
This is something that should be mentioned in the migration guide + maybe we should add an example for this to the insertable docs.

[toxeus]

@weiznich thank you for your detailed answer. I do appreciate that performance is also a priority in diesel.

This implies you cannot easily use a temporary value there, that is produced in the scope of ToSql::to_sql.

This is quite a breaking change for me. I have a lot of ToSql:to_sql implementations that rely on a temporary value for creating the database serialization similar how it's implemented for the datetime types.

If there is really a need for having this functionality there we can talk about adding a corresponding interface for this.

I think that there is definitely a need for supporting temporary values. The implementations for the datetime types are a proof for that need.

I'm open for suggestions here. To be clear here: Just making those functions pub is probably not the way to go. I would like to hear some possible alternatives here first.

I agree, making those functions pub is not ideal. Before, it was possible to generically implement ToSql::to_sql() with temporary values for any database backend. Simply exposing those functions will not restore that behavior. It would be great to be able to generically implement ToSql::to_sql() for any backend. That's why I asked about reborrow() for Sqlite. Using a smart pointer like Cow also yields to the same issues. Looks like a dead end to me.

[weiznich]

This is quite a breaking change for me. I have a lot of ToSql:to_sql implementations that rely on a temporary value for creating the database serialization similar how it's implemented for the datetime types.

While this is unfortunate, it's at least expected. Thats the reason why there will be a 2.0 release and why I'm telling people not to use the "master" branch as "stable" target.

I agree, making those functions pub is not ideal.

Happy to see proposals here. Otherwise this is something I do not consider blocking for the 2.0 release as I believe whatever interface we come up here can be later added on the top of the existing interface. A good starting point for such a discussion would be to collect cases where a temporary value seems to be necessary. From the refactoring of diesel itself it seems like most cases can be actually rewritten to support all backends with the new framework. That requires knowledge of the exact cases.

It would be great to be able to generically implement ToSql::to_sql() for any backend. That's why I asked about reborrow() for Sqlite. Using a smart pointer like Cow also yields to the same issues. Looks like a dead end to me.

To be clear here: That will likely never work again as the backends are just really different in how they handle binding values. The old sqlite implementation hide that from you by doing really nasty things that involved coping around values at least 3 times. That was really error prone and could lead to performance issues in some situations. The upside was that it looked like it "worked" the same way for outside users. Changing that was always something on our todo list. The 2.0 release seemed like a good opportunity to just do that.

[Ten0]

reborrow works fine for my purposes because I'm only on PG and unlikely to use SQLite any time soon. Thanks! :)
However it doesn't work for the same kind of implementations with SQLite.

If there is really a need for having this functionality there we can talk about adding a corresponding interface for this. I'm open for suggestions here. To be clear here: Just making those functions pub is probably not the way to go. I would like to hear some possible alternatives here first.

I have two in my post above. :)
The first one would probably look cleaner once we have GAT.

[weiznich]

@Ten0 To be the critic person here:

pub trait ToSql<'r, 'b, A, DB> {
   /// See the trait documentation.
   fn to_sql(&'r self, out: &mut Output<'b, '_, DB>) -> Result;
}

impl<'r, 'b, 'a: 'b, A, T, DB> ToSql<'r, 'b, A, DB> for &'a T
where
   T: ToSql<'a, 'b, A, DB> + ?Sized,
{
   fn to_sql(&'r self, out: &mut Output<'b, '_, DB>) -> Result {
       (*self).to_sql(out)
   }
}

impl<'r, 'b> ToSql<'r, 'b, BigInt, Pg> for i64 {
   fn to_sql(&'r self, out: &mut Output<'b, '_, Pg>) -> Result {
       out.write_val(*self)
   }
}
impl<'b, 'r: 'b> ToSql<'r, 'b, BigInt, Pg> for str {
   fn to_sql(&'r self, out: &mut Output<'b, '_, Pg>) -> Result {
       out.write(self)
   }
}

This would likely work to fix the problem with ToSql, but I see other problems here:

• It's at least unclear for me if this would be usable in the whole query building framework without requiring 'r: 'b at some point. Likely not. In that case we would continue to have basically the same problem that it's only possible to use impls that actually fulfill this contract.
• The "special trick" here seems to be the addition of write_val and write. That's not that different then just calling .reborrow() + the internal implementation from my point of view. For sqlite we need to have something different anyway as there is no concept like write there.
• Adding lifetimes to the trait itself is infectious. This would require adding lifetimes to basically all of diesels query builder traits + it would require having lifetimes in basically all generic code that down stream users will try to write. As this would make it even harder (or basically impossible) to write generic code involving diesel it would try to avoid this if possible.

pub trait ToSqlStatic<A, DB> {
   fn to_sql(&self, out: &mut Output<'_, '_, DB>) -> Result;
}
impl<T, A, DB> ToSql<A, DB> for T
where
   T: ToSqlStatic<A, DB>,
{
   fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, DB>) -> Result {
       <T as ToSqlStatic<A, DB>>::to_sql(self, out)
   }
}

That would require a lot more additional trait impls for all backends. Additionally implementing a custom type becomes more work. Although it would likely solve this specific problem.

---

I've played in the last days a bit with updating diesel-oci to incooperate the bind system rewrite. It turns out that this change is actually useful there and it seems like it will use a similar bind system than sqlite. That's a indicator for me that the new system is flexible enough to handle different usecases. That written I've noticed a few missing pieces to actually make this work:

• There is no way to deconstruct an Output later on from a third party crate. We may want to expose Output::into_inner() for this for third party backend crates.
• There is no way to set/access the value of the inner buffer if it does not implement Write. This might be required for custom backends that will bring their own Value enum.

Based on this situation I would like to summarize the current problem space here:

• The bind system rewrite did slightly change the requirements for implementing ToSql. It is not easily possible anymore to use temporary values as part of the ToSql impl. That's a potential problem for people upgrading their codebase as this is a "common" patterns for diesel 1.x ToSql impls. This implies we need to provide at least usable alternatives that are documented in the right places.
• For the Mysql/Postgresql backend (which use RawBytesBindCollector) there is the Output::reborrow() method which effectively fixes this issue. We need to make it more obvious that this methods solves problems with temporary values constructed inside of ToSql::to_sql by maybe adding an explicit example to ToSql::to_sql + adding a paragraph about this to the migration guide.
• For the Sqlite backend there is currently no way to construct and use a temporary value as part of the ToSql::to_sql implementation. Internally we solve this by using various private set_x functions that allow to set the corresponding value. We don't want to expose this functions. Additionally the Sqlite needs to differentiate between temporary and borrowed values.
• Third party backends do not really have the possibility yet to provide a custom buffer type for the BindCollector impl, as there is no way to access the buffer from the outside. They may have similar requirements as the Sqlite backend on how bind values should be restricted.

To solve those issues I propose the following solution:

• We add the following function to Output:

    pub fn set_value<V>(&mut self, value: V)
    where
        V: Into<<crate::backend::BindCollector<'a, DB> as BindCollector<'a, DB>>::Buffer>,
    {
        self.out = value.into();
    }

• We wrap SqliteBindValue<'a> into a struct to make the enum opaque for outside users and provide a set of From<_> impls for the newly exposed wrapper. That's the only way to construct the "enum".
• We replace any usage of Output::set_x() internally with the newly created set_value function
• (We expose Output::into_inner for third party backend)

This allows:

• Postgres/Mysql users can use reborrow() to "fix" the lifetime for their temporary values
• Sqlite users to construct and use temporary values as part of their ToSql::to_sql implementation
• It provides us an extensible way to construct the internal bind value enum
• Third party backends can either use the write based interface or provide their own buffer type + corresponding From<_> impls
• It does not introduce additional traits or lifetimes

Comments (and PR's) welcome 😉

[weiznich]

I've opened #2998 to address this

[toxeus]

While this is unfortunate, it's at least expected. Thats the reason why there will be a 2.0 release and why I'm telling people not to use the "master" branch as "stable" target.

Good to know that this is expected. It is my deliberate choice to use master for two reasons:

1. I'm depending on some of the 2.0 features
2. I want to track breaking changes such that I can discuss/report them before the release, e.g. this one

I'm fully aware that by doing that I need to deal with changing APIs. But I'd need to do that too if I would use 1.X releases and then migrate to 2.0 once it's released but without the possibility to migrate gradually and without being able to understand the changes before they are released and therefore stable.

Otherwise this is something I do not consider blocking for the 2.0 release as I believe whatever interface we come up here can be later added on the top of the existing interface.

That is my feeling too.

A good starting point for such a discussion would be to collect cases where a temporary value seems to be necessary.

I'll try to find solutions for the existing ToSql impls and will report those use-cases that aren't possible to implement with the current API.

To be clear here: That will likely never work again as the backends are just really different in how they handle binding values.

Thanks for the clarification. If that's a deliberate choice then this is perfectly fine for me.

[weiznich]

I'm fully aware that by doing that I need to deal with changing APIs. But I'd need to do that too if I would use 1.X releases and then migrate to 2.0 once it's released but without the possibility to migrate gradually and without being able to understand the changes before they are released and therefore stable.

Thanks for this clarification ❤️. Such feedback on API changes is always welcome.
One outcome of this discussion is that I noticed that we should have a paragraph about this in our planed migration guide, so that users know how to handle this situation. This should probably just point to Output::reborrow for the postgres and mysql backend and outline the underlying issue for the sqlite backend. Contributions here are welcome, the Changelog already contains fragments of such a guide.

[toxeus]

I'll try to contribute that section to the guide. Hopefully, I can get around doing that before the Christmas break.

our planed migration guide

I conclude that document doesn't exist yet. Should it be created in ./guide_drafts?

[weiznich]

There are already a few fragments in our changelog: https://github.com/diesel-rs/diesel/blob/master/CHANGELOG.md?plain=1#L255-L332
Writing a "real" migration guide based on this is something that needs to be done before the release. I will likely start working on this as soon as we have released an 2.0.0-rc0, which hopefully happens next month or so.

[ensc]

What is the recommended way to write a generic implementation of to_sql() now? Previously it was possible to write a DB agnostic one. Now, it seems that there must be written one for postgres stylish backends and one for sqlite like ones.

I have a lot of code like

#[diesel(sql_type = diesel::sql_types::SmallInt)]
pub enum FileType {
    SSTATE  = 1,
    SOURCE  = 2,
    SCM     = 3,
    CICACHE = 4,
}

impl <DB> ToSql<SmallInt, DB> for FileType
where
    DB: Backend,
    i16: ToSql<SmallInt, DB>
{
    fn to_sql<W: Write>(&self, out: &mut Output<W, DB>) -> SerResult {
	(*self as i16).to_sql(out)
    }
}

Changing this to new api does not seem to be trivial (the "Beside of the changed signature, existing impls of this trait should remain unchanged in almost all cases" in the ChangeLog definitively does not hold ;)

[weiznich]

Due to lack of specialization in rust, it seems that the sqlite feature can not be active with other ones (the generic impl ToSql<Timestamp, DB> for Time conflicts with impl ToSql<TimeStamp, db::sqlite::Sqlite> for Time). Fortunately, uninteresting for my use cases either....

That's just not true. The impl given above does work for all backends at the same time. It does not need any kind of specialization. For cases where you need different ToSql impls you can differentiate the impls based on Backend::BindCollector, rustc will accept that.

[ensc]

For cases where you need different ToSql impls you can differentiate the impls based on Backend::BindCollector, rustc will accept that.

struct Foo(i16);

impl <DB> ToSql<SmallInt, DB> for Foo
where
    DB: Backend<BindCollector = diesel::query_builder::bind_collector::RawBytesBindCollector<DB>>,
    i16: ToSql<SmallInt, DB>,
{
    fn to_sql(&self, out: &mut diesel::serialize::Output<DB>) -> diesel::serialize::Result {
	let tmp = self.0 + 1;	// some conversion
	ToSql::<SmallInt, DB>::to_sql(&tmp , &mut out.reborrow())
    }
}

impl <'a, DB> ToSql<SmallInt, DB> for Foo
where
    DB: Backend<BindCollector = diesel::sqlite::connection::SqliteBindCollector<'a>>,
{
    fn to_sql(&self, out: &mut diesel::serialize::Output<DB>) -> diesel::serialize::Result {
	let tmp = self.0 + 1;	// some conversion
	out.set_value(tmp as i32);
	Ok(diesel::serialize::IsNull::No)
    }
}

fails here with

• "module connection is private" -> SqliteBindCollector does not seem to be part of the public api
• error[E0119]: conflicting implementations of trait diesel::serialize::ToSql<diesel::sql_types::SmallInt, _> for type Foo

[Ten0]

SqliteBindCollector does not seem to be part of the public api

That is on purpose: the second implementation does not need to be generic on DB, because it's only used on SQLite -> it can be written non-generically, e.g. https://docs.diesel.rs/master/diesel/serialize/trait.ToSql.html#examples, which solves both your issues.

As a side note, you have this trick that typically enables working around associated types disjointedness issues.

[ensc]

@Ten0 A specialization impl ToSql<SmallInt, Sqlite> for Foo was my first try but it failed due to the conflicting implementation error too.

I disable code now with #[cfg(not(feature = "sqlite"))]; it is ok for my application but for libraries which have to work with both sqlite and postgres features this will not be a solution.

[weiznich]

So I've played a bit with this and yes you cannot have one generic impl using a specific bind collector + one backend specific impl, which use a different bind collector. That seems to be a rustc limitation and something that couldn't be worked around with the associated types disjointedness trick mentioned about. The point is: This behaviour is not different than before, that's exactly what happens with diesel 1.4.x as well if you try to constrain the impls there with similar bounds. It isn't as obvious there as all default diesel backends use the same bind collector type, but it can happen there as well.

Now where do we go from here: In my opinion this issue is unfortunate but not release blocking as it can be worked around in third party crates by:

• Writing one general impl without using bind collector specific methods (By not using temporary values as part of the to_sql impl)
• Writing concrete impls per backend
• Using conditional compilation.

That does not mean that I will be opposed to improvements in this area, just that I by myself likely won't spend much time on improving this before cutting the next release.

[Sytten]

Thanks to all here for the information, I was totally lost when I updated my target commit.
I know v2 is not stable, but I also needed features on master and I was already targeting an older version of main.
For others, this link contains all the information: https://docs.diesel.rs/master/diesel/serialize/trait.ToSql.html#examples