Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Serde (de)serialization #115

Open
pimeys opened this issue Jan 14, 2021 · 11 comments
Open

Add Serde (de)serialization #115

pimeys opened this issue Jan 14, 2021 · 11 comments
Labels
help wanted Extra attention is needed kind/improvement An improvement to existing feature and code.

Comments

@pimeys
Copy link
Contributor

pimeys commented Jan 14, 2021

We could maybe have a nice interface for people using serde structs in their code. Let's see a few examples:

#[derive(Deserialize)]
struct Cat {
    id: i64,
    name: String
}

Now using the struct, we could deserialize rows directly to them in an efficient manner:

let mut client = tiberius::Client::connect(config, tcp.compat_write()).await?;
let stream = client.query("SELECT id, name FROM cats", &[&1i32, &2i32, &3i32]).await?;
let rows = streams.into_first_result().await?;

let cats = stream
    .by_ref()
    .map_ok(|row| row.deserialize::<Cat>())
    .try_collect()
    .await?;

The question is, should Tiberius do this, or should it be let for higher-level crates, such as Diesel?

@pimeys pimeys added help wanted Extra attention is needed kind/improvement An improvement to existing feature and code. labels Jan 14, 2021
@yatesco
Copy link

yatesco commented Feb 7, 2021

On first sight this looks appealing but in practice you're concreting column names in the database with the names in the JSON that is inevitably exported. In real life it would be more like:

#[derive(Deserialize)]
struct Cat {
    id: i64,
    #[serde(rename="someField")] // for JSON, but do I want "someField" in my database
    some_field: String
}

I guess you could go down the whole "value object" route but blergh.

BTW, I tend not to do either of these - I tend to store the serialised JSON in the database and use json_value etc. to expose indexed fields, so I'm not really the target audience for this.

I do see the appeal for this during onboarding and for more isolated/smaller projects, but decades of pain have taught me ORMapping almost always fail :-)

EDIT: If I wanted to do this, could I already achieve this with impl From<WhateverTiberiusRowTypeIs> for Cat {?

@pimeys
Copy link
Contributor Author

pimeys commented Feb 8, 2021

We don't yet have FromRow trait, only FromSql that's per-column. I wouldn't say no to a PR to implement that though, that would be quite useful for many users and not hard at all to write.

And, I agree with you @yatesco, I don't really feel the need for serde support in this crate.

@mlcruz
Copy link

mlcruz commented Apr 7, 2021

I have been working for a while on something similar

Right now i have a very simple working example for a derive macro that implements TryFrom<tiberius::Row> for some struct. I'm quite new to proc-macros, so its not that great, but works well enough for our internal usage and its better than manually getting each field.

#[derive(TiberiusRow)]
struct Foobar {
    foo: String,
    bar: bool,
    zar: Option<String>,
    xar: Option<bool>,
}

Expands to:

impl TryFrom<tiberius::Row> for Foobar {
    type Error = tiberius::error::Error;
    fn try_from(__row: tiberius::Row) -> Result<Self, Self::Error> {
        Ok(Self {
            foo: {
                __row
                    .try_get::<&str, &str>("foo")?
                    .expect(&{
                        let res = ::alloc::fmt::format(::core::fmt::Arguments::new_v1(
                            &["Failed to get field "],
                            &match (&"foo",) {
                                (arg0,) => [::core::fmt::ArgumentV1::new(
                                    arg0,
                                    ::core::fmt::Display::fmt,
                                )],
                            },
                        ));
                        res
                    })
                    .to_owned()
            },
            bar: {
                __row.try_get("bar")?.expect(&{
                    let res = ::alloc::fmt::format(::core::fmt::Arguments::new_v1(
                        &["Failed to get field "],
                        &match (&"bar",) {
                            (arg0,) => [::core::fmt::ArgumentV1::new(
                                arg0,
                                ::core::fmt::Display::fmt,
                            )],
                        },
                    ));
                    res
                })
            },
            zar: { __row.try_get::<&str, &str>("zar")?.map(|f| f.to_owned()) },
            xar: { __row.try_get("xar")? },
        })
    }
}

I wouldn't mind improving on the implementation (error handling, tests, field attributes, etc) and submitting a PR, but i'm not sure if a derive macro is the right way to do this.

@tomhoule
Copy link
Contributor

tomhoule commented Apr 8, 2021

Hi @micruz! I am working with database drivers all the times and have written a lot of Rust macro code. I implemented something similar with serde in quaint and with hindsight, I definitely think a custom macro the way you are doing it is better: it generates less code and it can live as a separate crate.

One design consideration is whether you get the columns by name, like in your example, or by position (forcing the struct fields to be in the same order as the values in your SELECT clause). By position adds this constraint, but it's faster. The diesel ORM does it that way for example.

I think something like that as a separate crate or an optional feature in tiberius would be great :)

@pimeys
Copy link
Contributor Author

pimeys commented Apr 8, 2021

See also how ScyllaDB handles the de-serialization. Not saying which idea is the best, but it's interesting to see people taking different approaches to this problem:

https://github.com/scylladb/scylla-rust-driver/blob/main/examples/select-paging.rs#L36

@pimeys
Copy link
Contributor Author

pimeys commented Apr 8, 2021

Also, what I'd like to see is that if we take the proc macro route, that we could instead of copying, move the data from the row to the struct. It can really slow up the execution if we clone in this macro, without the developer noticing...

@tomhoule
Copy link
Contributor

tomhoule commented Apr 8, 2021

Yep, or have the struct hold references to the contents of the row. struct MyCustomRow<'a> { ... }.

@mlcruz
Copy link

mlcruz commented Oct 22, 2021

Sorry for taking so long to give you guys a response, I'm almost done with my bachelor's degree and the last six months have been really painful for me.

I have been working on this on and off for a few months now, and right now i do believe i got something almost usable (we do use this in production everywhere, but it was lacking some functionality and tests).

One design consideration is whether you get the columns by name, like in your example, or by position (forcing the struct fields to be in the same order as the values in your SELECT clause). By position adds this constraint, but it's faster. The diesel ORM does it that way for example.

This is a very good point! I'm not very sure on which one (by key or by position) should be the default. For now 'by_name' is the default, but you can use an attribute to enable getting columns by position

#[derive(FromRow)]
#[tiberius_derive(by_position)]
pub struct RowByPosition {
    pub foo: Option<i32>,
    pub bar: i32,
}

is going to expand to something like

            RowByPosition {
            foo: row.try_get(0usize)?,
            bar: row
                 .try_get(1usize)?
                 .ok_or_else(|| tiberius::error::Error::Conversion(format!(r" None value for non optional field {}", "bar") )?
            }

I also use try_get for everything and propagate an error instead of unwraping. row.get() unwrapping and the lack of more detailed errors are a huge pain point for us, so i tried to make error handling a little nicer.

Also, what I'd like to see is that if we take the proc macro route, that we could instead of copying, move the data from the row to the struct. It can really slow up the execution if we clone in this macro, without the developer noticing...

This is a also very good point. I'm not sure what should be the default, but for now its possible to add an 'owned' attribute to enable this behaviour.

#[tiberius_derive(owned)]
pub struct RowOwned {
    pub foo: Option<String>,
    pub bar: String,
}

expands into:

    impl RowOwned {
        pub fn from_row(row: tiberius::Row) -> Result<Self, tiberius::error::Error> {
            let mut row_iter = row.into_iter();
            Ok(Self {
                foo: {
                    <String as tiberius::FromSqlOwned>::from_sql_owned(row_iter.next().ok_or_else(
                        || {
                            tiberius::error::Error::Conversion(
                                format!(r"Could not find field {} from column with index {}", "foo", "0")
                                .into(),
                            )
                        },
                    )?)?
                },
                bar: {
                    (<String as tiberius::FromSqlOwned>::from_sql_owned(row_iter.next().ok_or_else(
                        || {
                            tiberius::error::Error::Conversion(
                                format!(r"Could not find field {} from column with index {}", "bar", "1")
                                .into(),
                            )
                        },
                    )?)?)
                    .ok_or_else(|| {
                        tiberius::error::Error::Conversion(
                            format!(r"None value for non optional field  {} from column with index {}", "bar", "1").into(),
                        )
                    })?
                },
            })
        }
    }

Yep, or have the struct hold references to the contents of the row. struct MyCustomRow<'a> { ... }.

This is now the default behaviour. I also added an attribute to enable converting &str to String. Since strings do not implement FromSql, having an attribute to do this is reasonably ergonomic, and doing it like so still keeps everything explicit. Maybe this is a good enough solution for #168 ?

#[derive(FromRow)]
#[tiberius_derive(auto)]
pub struct FoobarAuto<'a> {
    pub foo: Option<i32>,
    pub bar: Option<&'a str>,
    pub auto: String,
    pub auto_opt: Option<String>,
}

See also how ScyllaDB handles the de-serialization. Not saying which idea is the best, but it's interesting to see people taking different approaches to this problem:

https://github.com/scylladb/scylla-rust-driver/blob/main/examples/select-paging.rs#L36

This is very nice, I'm working on implementing this behavior for derives on tuple structs.

I also added an rename_all attribute that works like serde so you don't have to have pascalCase struct fields.

#[derive(FromRow)]
#[tiberius_derive(rename_all = "PascalCase")]

pub struct Foobar<'a> {
    pub foo_bar: Option<i32>,
    pub bar_foo: Option<&'a str>,
}

We have being using this library in production on a bunch of services, its a huge helper being able to just derive FromRow and Serialize/Deserialize for some struct. The documentation is pretty lacking but it should be possible to understand whats happening by looking at the tests.

There is still a lot of work to be done and i will keep working on this (maybe get some interns to help), but it might be good enough for some use cases at this point. Thank you guys very much for the feedback, it was a huge help. I'm still unsure about how naming, apis and defaults should be done (and this is my first time doing something a little more complex with proc macros, thanks god for darling), so any feedback you guys have is greatly appreciated.

@jsouto18
Copy link

Is anyone working on this feature?

@pimeys
Copy link
Contributor Author

pimeys commented Jan 19, 2023

Not that I know of. Do you want to give it a try?

@jsouto18
Copy link

@pimeys What do you think of the implementation done on https://github.com/mobiltracker/tiberius-derive?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed kind/improvement An improvement to existing feature and code.
Projects
None yet
Development

No branches or pull requests

5 participants