Convert primary keys to serialize_as type

[Aethelflaed]

I've got a newtype on oxydized_money::Currency to implement ToSql/FromSql/...

I recently added a table with a composite primary key which included a currency column, and I'm expecting to be able to use it the same way I used another id.

#[derive(Debug, Queryable, Selectable, Identifiable)]
#[diesel(table_name = monthly_stats)]
#[diesel(primary_key(year, month, currency))]
#[diesel(check_for_backend(diesel::sqlite::Sqlite))]
pub struct MonthlyStats {
    pub year: i32,
    pub month: i32,
    #[diesel(deserialize_as = db::Decimal)]
    pub amount: Decimal,
    #[diesel(deserialize_as = db::Currency, serialize_as = db::Currency)]
    pub currency: Currency,
}

Everything seems fine, until I try to compile the following instruction:

        diesel::update(&*self)
            .set(monthly_stats::amount.eq(db::Decimal::from(self.amount)))
            .execute(conn)?;

error[E0277]: the trait bound SelectStatement<FromClause<monthly_stats::table>>: FilterDsl<expression::grouped::Grouped<expression::operators::And<expression::grouped::Grouped<expression::operators::Eq<monthly_stats::columns::year, expression::bound::Bound<diesel::sql_types::Integer, &i32>>>, expression::grouped::Grouped<expression::operators::And<expression::grouped::Grouped<expression::operators::Eq<monthly_stats::columns::month, expression::bound::Bound<diesel::sql_types::Integer, &i32>>>, expression::grouped::Grouped<expression::operators::Eq<monthly_stats::columns::currency, &oxydized_money::Currency>>>>>>> is not satisfied

Or as explained a few lines later:

note: required by a bound in update
pub fn update<T: IntoUpdateTarget>(source: T) -> UpdateStatement<T::Table, T::WhereClause> {
the trait diesel::Expression is not implemented for oxydized_money::Currency, which is required by &MonthlyStats: IntoUpdateTarget

I can fix that by implementing Identifiable and HasTable manually, or by compiling against the provided patch

It works because my type is Copy, but at the same time this isn't uncommon for id types.

When it's not copy, I get something like:

cannot move out of self.field which is behind a shared reference
move occurs because self.field has type MyType, which does not implement the Copy trait

Plus some advice on implementing Clone, if it's not already Clone.

I could maybe find a way of ensuring the type is Copy if you think it's better that way, but if we go this way we might as well relax the AsChangeset & Insertable restriction on serialize_as for the same reasons, so it would be a bigger change.

Tell me what you think

[weiznich]

Thanks for submitting this PR. I think this is a reasonable change, although I have some concerns around backward compatibility here. Depending on what the other members of the contributor team decide we might or might not move forward with this change.

If we move forward we likely want to have a changelog entry for this.

[weiznich]

I believe that might be seen as breaking change, although one could also argue that this is a bug fix. Pinging @diesel-rs/contributors to get some more opinions on this.

[kw217]

(I'm a recent Diesel reviewer, so don't give my opinion too much weight)
There are two changes here, right?

• One seems to me clearly a bugfix: for a composite primary key, we miss the &'ident / &self for all but the first element, leading to potential nonsense. We should just fix that.
• The other is less clear: we apply serialize_as when we didn't before. Clearly we "should" apply it, but could people be depending on this in some way? I don't have enough experience to know.

[Aethelflaed]

As far as I know, composite primary key already works (as shown in the tests), so this is not a fix. The &'ident / &self part was repeated for every element already, as per quote!'s interpolation.

Here I changed field_ty and field_name to be Vec<TokenStream> directly, so I can push what will be needed by the impl directly.

[weiznich]

I think we go with the current variant. I would like to see an Changelog entry for this before merging otherwise the code + test cases look ok.

[Aethelflaed]

I think we go with the current variant. I would like to see an Changelog entry for this before merging otherwise the code + test cases look ok.

I've added a changelog entry and rebased the PR

[weiznich]

Thanks for the update