fix: Decode and Encode derives (#1031)

[benluelo]

Reopening #2329

[asteinba]

Hey @benluelo,

seems like some checks failed because there is a unused import which is treated as an error. Can you fix that? I would love to see this merged as this issue bugs me since a while.

Happy to help out as well :)

[abonander]

It's fixed in main, it just needs rebased.

[asteinba]

I opened up a PR to your branch with the rebase on main :)

[asteinba]

@abonander FYI: Rebase is merged, checks are green :).

[asteinba]

@abonander Can you merge this :)?

[asteinba]

Don't want to bother but I would really love to see this merged :) Anything that is missing that I can assist with?

[asteinba]

@abonander last try before I stop bothering you: Is there anything missing or anything I can help with to get this merged? I would not like to see the PR dying again as @benluelo already reopened that and the value that this PR adds is huge (at least for me, I ran into this "bug" multiple times).

[jelacious]

I am going to second that this is a fairly huge PR in terms of usability and would like to see this merged.

Furthermore, the lack of response for a month for such a big project is concerning.

[benluelo]

@abonander any update?

[abonander]

Strictly speaking, this appears to be a potential breaking change as discussed by @jplatte in https://github.com/launchbadge/sqlx/issues/1031#issuecomment-775175392 because it changes lifetime obligations for field types:

If I understand correctly, the previous bounds would potentially allow, e.g. a type that only implemented Decode<'static, Postgres> to be used as it would bubble up the obligation to the derived impl. But after the change, it would require all fields to implement for<'decode> Decode<'decode, Postgres>, either implicitly (by invoking Decode::decode()) or explicitly (on generics).

Would this realistically affect anybody in practice? It seems unlikely, but unless we're confident that this is not a breaking change because code relying on the previous behavior wouldn't have compiled anyways, then it has to wait for 0.8.0.

[jplatte]

Actually no, I wasn't talking about lifetimes. I was talking about something like this:

#[derive(sqlx::Encode)]
struct Foo<T> {
    field: sqlx::types::Json<T>,
}

which currently works because it generates a sqlx::types::Json<T>: sqlx::Encode bound but will fail with this PR because it generates T: sqlx::Encode when actually T: serde::Serialize is required.

This is an actual regression in functionality, but IMO warranted because otherwise private field types leak into the public trait implementation. It's also how virtually all other derives work. See also serde(bound), which is serde's solution for this that does not implicitly leak private implementation details.

[abonander]

That is a much better point than I was trying to make. Adding a #[sqlx(bound)] sounds like a good idea.

[abonander]

@benluelo we are now merging breaking changes, so if you could rebase and fix the conflict that'd be great.

[benluelo]

squashed and rebased, should be good to go (barring any issues I missed post-rebase)

[joelawm]

Is there a update to this as this is a blocking issue for a lot of people that want to use composite types in postgres that have null values. If there is anything I can do to help let me know. @abonander