diesel: No way to express inconsistency when deserializing for `Queryable` data since #2182

Given this I would say it is not necessary to change Queryable::build to return a Result.

So it seems we could have a workaround, making this indeed not completely “necessary”.

Also I understand it may sometimes be better to put aside 1% of the cases when the large majority of the time this should never possibly return an error, in order to keep the concepts simpler and save the hassle of error handling to callers of this function.

However,

• I feel that failing to recompose fields into a structure because data that was stored is inconsistent with what was expected
  • is not a niche scenario, as it’s used in several places in our codebase
  • conceptually makes sense
• Unless I’m mistaken, this function is only ever called (which is not a lot, so wouldn’t make error handling really harder)
  • in other Queryable::build impls, where error handling would be reduced to adding an Ok and a ?
  • in the FromSqlRow::build_from_row impl
• Solving it through Queryable instead of working around it would allow for other great natural extensions, such as:
  • #[diesel(deserialize_as = "SomeType")] could be naturally extended to TryInto, allowing e.g. to write the following:

    #[derive(Queryable)]
    struct S {
        // There is a DB `CHECK` that guarantees this is always positive
        #[diesel(deserialize_as = "i32")]
        some_field: u32,
    }
    

  • #[diesel(deserialize_as = "SomeType")] could be naturally extended to structures, allowing e.g. to solve my previous problem in the following very diesel-internal-abstract and clean way:

    #[derive(Queryable)]
    #[diesel(deserialize_as = "RawMyType")]
    struct MyType {
        pub delayed_by_days: Option<usize>,
        pub delayed_by_months: Option<MonthDelay>,
        pub some_other_field: Option<f64>,
    }
    #[derive(Queryable)]
    struct RawMyType {
        delayed_by_days: Option<i32>,
        delayed_by_months: Option<i32>,
        post_month_rounding_delayed_by_days: Option<i32>,
        some_other_field: Option<f64>,
    }
    impl TryFrom<RawMyType> for MyType { ... }
    

• It would also (even without these last two additional extensions) already naturally reduce the amount of boilerplate: no more of this:

  let values = <(Option<i32>, (Option<i32>, Option<i32>), Option<f64>) as diesel::deserialize::FromSqlRow<
  			ST,
  			DB,
  		>>::build_from_row(row)?;
  

  as this is already what Queryable allows to abstract and make easy and clean. (When I create a custom FromSqlRow impl, I’m actually always rewriting the impl FromSqlRow for T where T: Queryable, just stripping the Ok).
• I don’t feel like forcing to remove genericity when encountering such a scenario is super comfortable
• This would probably ease transition to diesel 2.0:
  • No wrapping your head around this precise issue and being forced to make code non-generic
  • May use the occasion to rewrite a few pieces that broke using the above suggested #[diesel(deserialize_as = "SomeType")] improvements
• Because it’s always implemented using the macro, it would require update of a very little amount of code.

So while it may indeed not be absolutely “necessary”, I still feel like this use-case is legitimate, and that allowing Queryable::build to return a failure would still be the cleanest and correct approach to solving it.

Given all this, do you think:

• this is not the right approach? In which case, we’d love to understand why.
• this is the right approach but you don’t have the resources to implement/document it? In which case, my proposal to implement and open the PR still holds, and we’d also totally be willing to help with the documentation.

Thanks for your quick answer and for expressing your thoughts in such great detail! 🙂

First of all currently the main contract of Queryable is that it’s just a simple map from a tuple returned by the database to a struct, meaning that currently it is conceptually not the place where any type conversation should happen. Changing this is less a technical thing, more a broad conceptual change how things are supposed to work.

From the doc, I read “The purpose of this trait is to convert from a tuple of Rust values that have been deserialized into your struct.”, but I’m not sure where there is that conceptual constraint of if being “simple”, or “one-to-one” with the fields. Besides, another part of the code seems to say it’s “typically a tuple of all of your struct’s fields”, but could also be any other Rust type that can be deserialized from this SQL type. So the way I understand it, Queryable could be the concept of “defining deserialization through converting from an already-deserializable Rust type to another”.

That being said, even if it is a conceptual change, I think changing a concept can not break concept boundaries if both these properties are satisfied:

• Previous concept is included in new concept
• New concept still makes sense everywhere it’s used which (unless I miscounted the calls when writing my previous message) in our case seems to only be this impl, and recursive use which cannot ever not make sense with itself

#[diesel(deserialize_as = “SomeType”)] could be naturally extended to TryInto, allowing e.g. to write the following:

That’s a nice idea, but needs much more details. Open questions:

• What constraints are on the error type?

Error + Send + Sync, so it can be boxed into the error type used there: https://github.com/diesel-rs/diesel/blob/d7ce43d2130bca434cd1acc2943a0c944d1deba4/diesel/src/deserialize.rs#L12

• How would we handle the existing impls that use From? I wouldn’t like to break them.

Because the standard library implements TryFrom<U> for T where U: Into<T>, and because std::convert::Infaillible is Error + Send + Sync they would naturally not break and keep the same behaviour.

(I actually had these two answers in mind when suggesting this could be done, but I used the idea that the message was heavy enough already as an excuse for myself to not detail them. 😁)

#[diesel(deserialize_as = “SomeType”)] could be naturally extended to structures, allowing e.g. to solve my previous problem in the following very diesel-internal-abstract and clean way:

I’m honestly not sure if that is that much clearer/shorter/less boilerplate than just implementing FromSqlRow directly.

Not sure I understand: do you consider

let values = <(Option<i32>, (Option<i32>, Option<i32>), Option<f64>) as diesel::deserialize::FromSqlRow<
			ST,
			DB,
		>>::build_from_row(row)?;

to not be boilerplate, even though it has to be redundant with

where (Option<i32>, (Option<i32>, Option<i32>), Option<f64>): diesel::deserialize::FromSqlRow<ST, DB>,

?

Otherwise I don’t see what you mean, as clearly these lines disappearing is by definition less boilerplate and shorter, and the conversion just using TryInto definitely feels easier to grasp (especially for a new user) than all these diesel bounds and traits/types:

impl<ST, DB> diesel::deserialize::FromSqlRow<ST, DB> for MyType
where
	DB: diesel::backend::Backend,
	RawMyType: diesel::deserialize::FromSqlRow<ST, DB>,
{
	fn build_from_row<'a>(row: &impl diesel::row::Row<'a, DB>) -> diesel::deserialize::Result<Self> {

(When I create a custom FromSqlRow impl, I’m actually always rewriting the impl FromSqlRow for T where T: Queryable, just stripping the Ok).

That’s because of the different contracts of the traits. FromSqlRow stands for this type can probably constructed from any row coming from the database, while Queryable says you can map the result of a tuple of the sql type ST to the following datatype. That’s semantically different.

I’m aware of this, what I was trying to express is that the fact that I’m always rewriting most of what Queryable does when writing custom FromSqlRow impls highlights that this need actually fits very well with the Queryable concept, should we extend it a little.

Because it’s always implemented using the macro, it would require update of a very little amount of code.

At least that assumption is wrong. There are definitively use cases where Queryable is implemented directly. Such a change would have an additional migration impact on them.

Oh I might have mis-counted when writing my previous message. I had found (within diesel):

  * in other `Queryable::build` impls, where error handling would be reduced to adding an `Ok` and a `?`

and the code of the macro itself. Where else ?

Or do you mean Queryable is manually implemented in several places in external crates ? In which case I’m surprised because that is not at all something we’ve been using precisely because it wouldn’t be allowed to fail to build these structures, so it doesn’t extend our capabilities compared to deriving Queryable and selecting the appropriate fields in the right place.

This would probably ease transition to diesel 2.0:

This definitively needs more explanation on how this could make a transition easier. To use that code using FromSqlRow must definitively be rewritten, while currently code having a non generic impl only needs some minor fixes for the signature of build_from_row. My general motivation is to keep the breakage of updating to 2.0 as small as possible. If possible normal application code that uses the dsl, the derives and some advanced features should not break at all.

I don’t see how the change I suggest changes anything to how FromSqlRow is declared or used, or to its concept. If you meant “code using Queryable” instead, then yes it would require some change, however it would be “as small as possible” as per the question just before, and may be compensated by the fact we provide a clean solution for what used to be the custom FromSqlRow impls. (At least for us, that would have been worth).

I don’t feel like forcing to remove genericity when writing encountering such a scenario is super comfortable

Would you mind giving a example where this genericity is required? I think 99% of the impl can be non-generic or use different concrete impls based on their needs.

I’m not saying I have a concrete use-case where genericity is actually required, I’m saying when I read code that mentions “I only work with this specific backend and these specific SQL types”, I tend to wonder why it wouldn’t work otherwise, question on which I could spend a rather unreasonable amount of time if the answer is “because under the strict condition of non genericity, Rust will not trigger conflicting implementation errors of the kind downstream crates may implement trait”, which means we would have to have comments describing this issue instead of having a code that’s self-explanatory. Not to mention the fact we’d have to manually match the Rust types with their SQL counterparts everytime we’d work on such an impl.

That all written: I’m not fundamentally opposed such an idea, but I think we should put a bit more though in the design before just changing things.

Which is what we’re doing right now, that’s great, thanks a lot 😊

I do not want to issue a 3.0 release soon after 2.0, just because we messed something up.

Same applies in the opposite direction. (😜 Joke, I see how this is completely linked to your previous sentence and I completely agree we should always be careful. It’s true though.)

Another more fundamental question: If we change Queryable to return a Result, what would be the technical difference between Queryable and FromSqlRow?

Both the technical and conceptual difference would be that FromSqlRow is responsible for the whole process of recomposing the structure from the diesel/database Rows, while Queryable is the part of that FromSqlRow process responsible for the Rust-to-Rust reorganization/validation part of it, independently of how the SQL types are naturally and fail-safely (provided valid schema) converted to Rust types.

Implementing Queryable instead of FromSqlRow would allow (and already allows) to easily safely abstract out of the SQL-to-Rust conversions, provided a valid schema.

From a technical point neither Querable nor QueryableByName are required. Both derives can just generate the corresponding FromSqlRow impl. I did this already as part of the type system rewrite, but added both traits back out of compatibility concerns with existing generic code.

I think both would make even more sense if they allowed for easy extra validation.

---

Aaaand that’s it! 😅 I was already writing answers to your previous message for a while when your last one came in, and it’s already been an hour since then. 😄 What do you think ?