No way to express inconsistency when deserializing for Queryable data since #2182
#2523
Comments
|
So as first thing I want to write: At least for me it is not that clear that the expected output here should be that this actually compiles. Those impls are in place because otherwise diesel wouldn't work that way it works now. I do not see any way to remove them without causing an even wider breakage. That does not mean that I'm opposed in improving the situation, but at least now I don't have any idea how and I do not have the time anymore to spend another month on figuring out how to write that trait bounds. If someone has an idea, feel free to open a PR. That written: At least the "ownstream crates may implement trait" error goes away if you use a concrete type for |
Haha you're right! I probably should have removed the "Expected output" section from the template. ^^ Would you be open to a such PR? diesel/diesel/src/deserialize.rs Lines 146 to 157 in d7ce43d diesel/diesel/src/deserialize.rs Lines 248 to 255 in d7ce43d diesel/diesel/src/deserialize.rs Lines 314 to 316 in d7ce43d diesel/diesel/src/deserialize.rs Lines 388 to 399 in d7ce43d
Hmm this workaround doesn't seem to work for me. (Maybe because we're also generic on |
Ten0
changed the title
FromSqlRow: downstream crates may implement trait QueryableByName/QueryableQueryable data since #2182
I've just checked that. If every involved type is a concrete type, you can implement As additional unrelated note: It would be great to get some feedback which kind of changes are required to update from the last diesel release to diesel 2.0/master, as this is definitively something we should somehow mention in our changelog/update guide. (Something that needs to be written…) |
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,
So while it may indeed not be absolutely "necessary", I still feel like this use-case is legitimate, and that allowing Given all this, do you think:
|
|
I will use the weekend to think about this proposal. In the mean time here a few short points that should definitively be addressed: First of all currently the main contract of
That's a nice idea, but needs much more details. Open questions:
I'm honestly not sure if that is that much clearer/shorter/less boilerplate than just implementing
That's because of the different contracts of the traits.
At least that assumption is wrong. There are definitively use cases where
This definitively needs more explanation on how this could make a transition easier. To use that code using
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. 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. I do not want to issue a 3.0 release soon after 2.0, just because we messed something up. |
|
Another more fundamental question: If we change From a technical point neither |
|
Thanks for your quick answer and for expressing your thoughts in such great detail! 🙂
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, 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:
diesel/diesel/src/deserialize.rs Line 12 in d7ce43d
Because the standard library implements (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. 😁)
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 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> {
I'm aware of this, what I was trying to express is that the fact that I'm always rewriting most of what
Oh I might have mis-counted when writing my previous message. I had found (within diesel):
and the code of the macro itself. Where else ? Or do you mean
I don't see how the change I suggest changes anything to how
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
Which is what we're doing right now, that's great, thanks a lot 😊
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.)
Both the technical and conceptual difference would be that Implementing
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. 😄 |
My main pain point here is that
There are other reasons that doing actual failable deserialization work why a manual
Just to link at least one: Example from
Just to write that down as explicitly as possible. Even if those impls seem to be generic, practically they are not generic. (At least from the point of an application, for libraries that build on top of diesel this is a bit different):
It's not required to do this match manually. In most cases you can just use
Sure those lines are boilerplate, but I fail to see how this does become better with using As we are trying to design a new/better API here, I will just write down different variants how this could be implemented explicitly. This hopefully should make it much clearer what I mean here. Let's start with that variant that's matches your impl above and can be written today with the master branch. struct MyType {
pub delayed_by_days: Option<usize>,
pub delayed_by_months: Option<MonthDelay>,
pub some_other_field: Option<f64>,
}
impl FromSqlRow<(Nullable<Integer>, (Nullable<Integer>, Nullable<Integer>), Nullable<Double>), diesel::pg::Pg> for MyType
{
fn build_from_row<'a>(row: &impl diesel::row::Row<'a, diesel::pg::Pg>) -> diesel::deserialize::Result<Self> {
let values = <(Option<i32>, (Option<i32>, Option<i32>), Option<f64>) as diesel::deserialize::FromSqlRow<
(Nullable<Integer>, Nullable<Integer>), Nullable<Double>),
diesel::pg::Pg,
>>::build_from_row(row)?;
Ok(Self {
delayed_by_days: values.0.map(|delay| delay.try_into()).transpose()?,
delayed_by_months: match values.1 {
(Some(nb_months), day_of_month) => Some(MonthDelay {
nb_months: nb_months.try_into()?,
post_rounding_delay: day_of_month.map(<_>::try_into).transpose()?,
}),
(None, None) => None,
(None, Some(_)) => {
return Err("Failed to deserialize MonthDelay, nb_months null and day_of_month not null".into())
}
},
some_other_field: values.2.filter(|&float| float != 0.),
})
}
}So the only thing that changes to the original impl is that we removed all generic arguments. On the one hand this skips a part of the complex where clause, on the other hand this removes some genericity. So let's analyze what's exactly is not possible anymore for this impl:
As next version let's have a look on how to write this impl using a possible version of diesel that returns a struct MyType {
pub delayed_by_days: Option<usize>,
pub delayed_by_months: Option<MonthDelay>,
pub some_other_field: Option<f64>,
}
impl<ST, DB> Queryable<ST, DB> for MyType
where (Option<i32>, (Option<i32>, Option<i32>), Option<f64>): Queryable<ST, DB>,
DB: Backend
{
type Row = <(Option<i32>, (Option<i32>, Option<i32>), Option<f64>) as Queryable<ST, DB>>::Row;
fn build(row: Self::Row) -> diesel::deserialize::Result<Self> {
let values = <(Option<i32>, (Option<i32>, Option<i32>), Option<f64>) as Queryable<
ST,
DB,
>>::build(row)?;
Ok(Self {
delayed_by_days: values.0.map(|delay| delay.try_into()).transpose()?,
delayed_by_months: match values.1 {
(Some(nb_months), day_of_month) => Some(MonthDelay {
nb_months: nb_months.try_into()?,
post_rounding_delay: day_of_month.map(<_>::try_into).transpose()?,
}),
(None, None) => None,
(None, Some(_)) => {
return Err("Failed to deserialize MonthDelay, nb_months null and day_of_month not null".into())
}
},
some_other_field: values.2.filter(|&float| float != 0.),
})
}
}While this closely matches your original impl, this does in my opinion not simplify the impl in any meaningful way. It's basically just the same as before. Therefore I think just changing the Next let's have a look at a version where we have a struct wide #[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 {
type Error = Box<dyn Error + Send + Sync>;
fn try_from(v: RawMyType) -> Result<Self, Self::Error> {
Ok(Self {
delayed_by_days: v.delayed_by_days.map(|delay| delay.try_into()).transpose()?,
delayed_by_months: match (v.delayed_by_months, v.post_month_rounding_delayed_by_days) {
(Some(nb_months), day_of_month) => Some(MonthDelay {
nb_months: nb_months.try_into()?,
post_rounding_delay: day_of_month.map(<_>::try_into).transpose()?,
}),
(None, None) => None,
(None, Some(_)) => {
return Err("Failed to deserialize MonthDelay, nb_months null and day_of_month not null".into())
}
},
some_other_field: v.some_other_field.filter(|&float| float != 0.),
})
}
}While you would not have some "magic" line to construct our initial tuple here, we need to list all field of the struct not only once, but three times, as you need to write the definition for Taking a step back at the first impl, as you can write it at least a bit clearer as your example: struct MyType {
pub delayed_by_days: Option<usize>,
pub delayed_by_months: Option<MonthDelay>,
pub some_other_field: Option<f64>,
}
impl FromSqlRow<dsl::SqlTypeOf<YourQuery>, diesel::pg::Pg> for MyType
{
fn build_from_row<'a>(row: &impl diesel::row::Row<'a, diesel::pg::Pg>) -> diesel::deserialize::Result<Self> {
use diesel::row::NamedRow;
Ok(Self {
delayed_by_days: row.get::<Nullable<Integer>, _>("first_field_name").map(|delay| delay.try_into()).transpose()?,
delayed_by_months: match row.get::<Nullable<Integer>, _>("second_field_name") {
(Some(nb_months), day_of_month) => Some(MonthDelay {
nb_months: nb_months.try_into()?,
post_rounding_delay: day_of_month.map(<_>::try_into).transpose()?,
}),
(None, None) => None,
(None, Some(_)) => {
return Err("Failed to deserialize MonthDelay, nb_months null and day_of_month not null".into())
}
},
some_other_field: row.get::<Nullable<Double>, _>("other_field").filter(|&float| float != 0.),
})
}
}As you can receive all values directly form the row, there is not really a need to use that line you consider as boilerplate to construct the values anymore. (In that context, I probably meaningful addition to our API would probably to have a To summarize that what I've written a bit: I think the underlying problem is that we don't have a great way to express that some additional deserialisation work is required for a specific field (or even for combining multiple fields). That's definitively something that can and should be improved, but at least the current suggestions are in my opinions not really a great fit here. It feels like there must be a better option, that just allows to say how to map this specific subset of fields without touching all the unrelated fields. Unfortunately I do not have any concrete idea how such an API could look like. One possible option would be to fit this somewhere in between calling |
|
Thanks again for your quick and detailed answer.
First, I'd like to underline that I'm very aware that
it seems like the part setting Regarding the point of concepts being the same, I think you had already expressed it in #2523 (comment), and I thought it had been addressed by:
as well as the first paragraph you were answering to, that was coming to the conclusion that even now,
It looks to me that one concept is included in (in the sense that it is meant to be executed as part of, and is never called out of) the other, both on the current master as well as in this suggestion, but that in both cases they still differ.
I love this example because I could not have given a better one advocating for my suggestion: this is precisely the kind of problem my suggestion is meant to solve. What happened there seems to be:
Because we are looking for high availability and reliability even if there happened to be different levels of experience among coders, it's not acceptable for us to panic in these cases. In fact, it looks like a large majority of the case, panicking is not going to be the appropriate answer, while an
Or any other kind of operation that cannot fail => Then one day you have a new scenario, it can start failing, and you have to rewrite it using a different trait, drop genericity, figure out the SQL types mapping, and add some more boilerplate. My bet is a lot of people are going to just add Regarding the comments on the amount of boilerplate:
So it looks like with these criteria, the
This I beleive had been mentionned in #2523 (comment):
The issue I tried to explain at #2523 (comment) wasn't that it couldn't be written or wouldn't work, it was that code mentioning it isn't generic when in fact it could be implies that a reader may wonder "why it couldn't be generic", "whether it relies on some kind of specific postgres internals", etc. Overall I feel like this would raise unnecessary questions when trying to understand the given code than if code just said "this works for whatever as long as I can deserialize these fields already". And unnecessary questions raised is lost time. That being said, I admit it's a relatively minor part of why I feel that code using Now I'm not sure how
addresses this point of "it makes code harder to understand", however it does raise a very interesting question: if having different pub trait Queryable
{
/// The Rust type you'd like to map from.
///
/// This is typically a tuple of all of your struct's fields.
type Row;
fn build(row: Self::Row) -> Self; // or Result<Self>
}
impl<T, ST, DB> FromSqlRow<ST, DB> for T
where
T: Queryable,
ST: SqlType,
DB: Backend,
T::Row: FromStaticSqlRow<ST, DB>,
{
fn build_from_row<'a>(row: &impl Row<'a, DB>) -> Result<Self> {
let row = <T::Row as FromStaticSqlRow<ST, DB>>::build_from_row(row)?;
Ok(T::build(row))
}
} This would in turn simplify again: impl Queryable for MyType
{
type Row = (Option<i32>, (Option<i32>, Option<i32>), Option<f64>);
fn build(row: Self::Row) -> diesel::deserialize::Result<Self> {
Ok(Self {
delayed_by_days: values.0.map(|delay| delay.try_into()).transpose()?,
delayed_by_months: match values.1 {
(Some(nb_months), day_of_month) => Some(MonthDelay {
nb_months: nb_months.try_into()?,
post_rounding_delay: day_of_month.map(<_>::try_into).transpose()?,
}),
(None, None) => None,
(None, Some(_)) => {
return Err("Failed to deserialize MonthDelay, nb_months null and day_of_month not null".into())
}
},
some_other_field: values.2.filter(|&float| float != 0.),
})
}
}while additionally increasing the distinction with from
It looks like this #[derive(Queryable)]
struct S {
specific_subset: SpecificSubset,
unrelated_fields: i32,
}
struct SpecificSubset {
...
}
impl Queryable for SpecificSubset {
type Row = ...;
fn build(row: Self::Row) -> diesel::deserialize::Result<Self> {
...
}
}Of course I renew my offer to spend some time implementing this and experimenting with it. 🙂 Thanks again for your great answer. |
So the main reason why (Additionally such a change would prevent having more than one
Feel free to experiment a bit with this. Here are the constraints from my side for accepting something like this in diesel:
I think a potential way to solve this would be a three step approach:
This would require having not one, but two associated Minor unrelated notes(Just want to have some correction here, for the case someone looks at this later and uses this as source for whatever…)
(that's only a minor nit)
As written in my original comment: I would happily accept a PR adding a |
IIRC this was already done as part of #2182: Line 30 in d7ce43d Lines 53 to 55 in d7ce43d However that seems even more risky to use than the tuple, in terms of "code will not crash at runtime". |
|
I'm having a bit of trouble reading the whole answer #2523 (comment) as there seem to be a lot of quotes from my previous message appearing in seemingly random places (middle of sentences/words).
I'm not sure why you separate 2 and 3 in your suggestion: often, struct constructors will perform validation (e.g. mod a_greater_than_b {
#[derive(Copy, Clone, Error)]
struct AGreaterThanBBuildError;
#[derive(Copy, Clone)]
struct AGreaterThanB {
a: i32,
b: i32,
}
impl AGreaterThanB {
fn new(a: i32, b: i32) -> Result<Self, AGreaterThanBBuildError> {
if a >= b {
Ok(Self{ a, b })
} else {
Err(NotPositive)
}
}
fn into_inner(self) -> (i32, i32) {
(self.a, self.b)
}
}
}
// Where it's db-checked to always be the case for whichever types we select in this
impl Queryable for AGreaterThanB {
type Row = (i32,i32);
fn build((a,b): Self::Row) -> diesel::deserialize::Result<Self> {
Ok(AGreaterThanB::new(a, b)?)
}
}What do you mean by them being splitted, why do you think they should be splitted, and how would you write this if they were splitted?
I don't agree it should not be named Queryable: I feel like the concept as I was defining it still matches with the current spirit and documentation of Queryable (though its behaviour changes a bit in that it tolerates failure when performing the type conversions): diesel/diesel/src/deserialize.rs Lines 14 to 17 in d7ce43d
Yes, I thought this was addressed by:
|
Sorry for that, should be fixed now.
That method returns a
Just to reiterate that: All
I think that will help to reduce the impact on already existing code. If somehow possible I just want to prevent that existing code is broken, which would be the case if we rerurn a Result from Queryable::build. (At least as far as this does not result into a bad API) |
Right. It looks like the lines still disappear though if we just write
Almost, there's still one in the 1st paragraph ^^
Oh yes it looks like it would be easy to extend this: Lines 191 to 205 in d7ce43d though it probably shouldn't be called NamedRow anymore then. Maybe just a new method on the standard Row?
Still that would be unsatisfactory for the amount of type-safety I'd like to achieve:
It looks to me like the
I really feel that it's quickly worth forcing people to update their code by adding an I think it would be reasonably easy to update with this detailed in the |
|
Again, feel free to do some experimentation here. As already expressed above I have a different opinion about changing anything about the definition of Pinging @diesel-rs/contributors here, maybe someone has other opinions here. |
So just to confirm with examples, you think if we choose to allow Diesel 2.0 to force users to update their existing manual implementations of impl diesel::deserialize::Queryable<(diesel::sql_types::Integer, diesel::sql_types::Text, diesel::pg::types::sql_types::Jsonb), diesel::pg::Pg> for Badge {
type Row = (i32, String, serde_json::Value);
fn build((_, badge_type, attributes): Self::Row) -> Self {
let json = json!({"badge_type": badge_type, "attributes": attributes});
serde_json::from_value(json).expect("Invalid CI badge in the database")
}
}to impl diesel::deserialize::FromSqlRow<(diesel::sql_types::Integer, diesel::sql_types::Text, diesel::pg::types::sql_types::Jsonb), diesel::pg::Pg> for Badge {
fn build_from_row<'a>(row: &impl diesel::row::Row<'a, diesel::pg::Pg>) -> diesel::deserialize::Result<Self> {
let values = <(i32, String, serde_json::Value) as diesel::deserialize::FromSqlRow<
(diesel::sql_types::Integer, diesel::sql_types::Text, diesel::pg::types::sql_types::Jsonb),
diesel::pg::Pg,
>>::build_from_row(row)?;
let json = json!({"badge_type": badge_type, "attributes": attributes});
Ok(serde_json::from_value(json).map_err(|e| format!("Invalid CI badge in the database: {}", e))?)
}
}(or from impl<ST, DB> diesel::deserialize::Queryable<ST, DB> for Badge
where
DB: diesel::backend::Backend,
(i32, String, serde_json::Value): diesel::deserialize::FromSqlRow<ST, DB>,
{
type Row = (i32, String, serde_json::Value);
fn build((_, badge_type, attributes): Self::Row) -> Self {
let json = json!({"badge_type": badge_type, "attributes": attributes});
serde_json::from_value(json).expect("Invalid CI badge in the database")
}
}impl<ST, DB> diesel::deserialize::FromSqlRow<ST, DB> for Badge
where
DB: diesel::backend::Backend,
(i32, String, serde_json::Value): diesel::deserialize::FromSqlRow<ST, DB>,
{
fn build_from_row<'a>(row: &impl diesel::row::Row<'a, diesel::pg::Pg>) -> diesel::deserialize::Result<Self> {
let values = <(i32, String, serde_json::Value) as diesel::deserialize::FromSqlRow<
ST,
DB,
>>::build_from_row(row)?;
let json = json!({"badge_type": badge_type, "attributes": attributes});
Ok(serde_json::from_value(json).map_err(|e| format!("Invalid CI badge in the database: {}", e))?)
}
}) impl diesel::deserialize::Queryable for Badge {
type Row = (i32, String, serde_json::Value);
fn build((_, badge_type, attributes): Self::Row) -> diesel::deserialize::Result<Self> {
let json = json!({"badge_type": badge_type, "attributes": attributes});
Ok(serde_json::from_value(json).map_err(|e| format!("Invalid CI badge in the database: {}", e))?)
}
}despite the fact that it would probably as you mentioned require changing the idiomatic
If that is the case then it looks like I'm mistaken in my above assumption that a mismatch between the SQL types |
|
I just used some time to think about this some more: (You probably know most of those things already, but I find it important to write this somewhere down, so that other people can understand how diesel works internally) I should probably first say that I'm not really happy about how the current deserialization structure works. The old (diesel 1.x) implementation required That all written now some rough overview how the old and new deserialisation implmentation work: Old implementation (1.x)We had basically two implementation:
|
|
So I've experimented with a bit with something in the spirit of your last suggestion:
Do you have a particular idea on how to implement this part considering that In order to solve this I've spent some time trying to implement something using an associated type to determine whether to use the initial row or a given type as argument to Because of this, and because it feels simpler conceptually than having a process where we enforce to have several steps, I like a lot the model where
That is true if
Yes I like a lot the So I've made an experiment with the following model, available at #2559: pub trait FromSqlRow<ST, DB: Backend>: Sized {
/// See the trait documentation.
fn build_from_row<'a>(row: &impl Row<'a, DB>) -> Result<Self>;
}
pub trait Queryable
{
/// The Rust type you'd like to map from.
///
/// This is typically a tuple of all of your struct's fields.
type Row;
/// Construct an instance of this type
fn build(row: Self::Row) -> deserialize::Result<Self>;
}
impl<T, ST, DB> FromSqlRow<ST, DB> for T
where
T: Queryable,
ST: SqlType,
DB: Backend,
T::Row: FromStaticSqlRow<ST, DB>,
{
fn build_from_row<'a>(row: &impl Row<'a, DB>) -> Result<Self> {
let row = <T::Row as FromStaticSqlRow<ST, DB>>::build_from_row(row)?;
Ok(T::build(row))
}
}
// Not implemented in the PR yet but it's pretty obvious this would work
trait Row<'a, DB> {
fn get<ST, T, I>(&self, idx: I) -> Option<deserialize::Result<T>>
where T: FromSql<ST, DB>,
Self: RowIndex<I>,
// field_count and partial_row as already defined
}I've noticed making An alternate approach I haven't experimented with but I think I'd be pretty happy with would be to use a macro to enforce implementation constraints on diesel/diesel/src/deserialize.rs Lines 379 to 382 in ff8e5b1 pub trait FromSqlRow<ST, DB: Backend>: Sized {
/// See the trait documentation.
fn build_from_row<'a>(row: &impl Row<'a, DB>) -> Result<Self>;
}
trait Row<'a, DB> {
fn get<ST, T, I>(&self, idx: I) -> Option<deserialize::Result<T>>
where T: FromSql<ST, DB>,
Self: RowIndex<I>,
// field_count and partial_row as already defined
}
// and e.g.:
queryable! {
fn build(something: SomeOtherFromSqlRowType) -> diesel::deserialize::Result<WhatWeWantToImplFromSqlRowOn> {
...
}
}
// which would generate aprox. this:
fn build(something: SomeOtherFromSqlRowType) -> diesel::deserialize::Result<WhatWeWantToImplFromSqlRowOn> {
...
}
impl<ST, DB> diesel::deserialize::FromSqlRow<ST, DB> for WhatWeWantToImplFromSqlRowOn
where
DB: diesel::backend::Backend,
SomeOtherFromSqlRowType: diesel::deserialize::FromSqlRow<ST, DB>,
{
fn build_from_row<'a>(row: &impl diesel::row::Row<'a, diesel::pg::Pg>) -> diesel::deserialize::Result<Self> {
let values = <SomeOtherFromSqlRowType as diesel::deserialize::FromSqlRow<
ST,
DB,
>>::build_from_row(row)?;
Ok(build(values)?)
}
}This would make it more complex for people who have custom |
One approach to fixing diesel-rs#2523
Setup
Versions
Problem Description
Since #2480 was fixed, I've just been giving another try at updating our codebase to the latest master (#2182).
It turns out I still couldn't because I can't define custom
FromSqlRowimpls, due to conflicting impls with downstream crates may implement trait, where Rust claims that Diesel could go ahead and implementQueryableorQueryableByNamethemselves on my type, which would create conflicts because of the following impls:diesel/diesel/src/deserialize.rs
Lines 359 to 367 in d7ce43d
diesel/diesel/src/deserialize.rs
Lines 388 to 399 in d7ce43d
Btw, same error when trying to implement
StaticallySizedRowbecause ofdiesel/diesel/src/deserialize.rs
Lines 435 to 442 in d7ce43d
What are you trying to accomplish?
I want to customize a
FromSqlRowImplimpl because I want to be allowed to fail on deserialization when seeing a data inconsistency, as well as easily reorganize my data in different sub-structures at the same time, while performing validation (whichQueryabledoes not allow me to do).What is the expected output?
compiles
What is the actual output?
Steps to reproduce
e.g. (for the above error)
The text was updated successfully, but these errors were encountered: