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

Custom implementations of sqlx Encode and Decode traits for db data types? #89

Closed
sandreae opened this issue Apr 23, 2022 · 13 comments
Closed

Comments

@sandreae
Copy link
Member

Would this be a neat way of replacing EntryRow which contains mostly untyped strings with a struct containing typed fields which can still be passed in and out of the dB?

Like this:
https://github.com/launchbadge/sqlx/blob/b6e127561797fe9aababa24ec640275ecb9b42af/sqlx-core/src/sqlite/types/str.rs

@sandreae sandreae changed the title Custom implementations of sqlx Encode and Decode traits for SQLite dB types? Custom implementations of sqlx Encode and Decode traits for db data types? Apr 23, 2022
@adzialocha
Copy link
Member

This is how we did it before in p2panda_rs but I didn't like having sqlx related code in the core library.

Is there a way to do it in aquadoggo?

@sandreae
Copy link
Member Author

Ah yeh, I remember that change. I wrote this meaning we would implement them in aquadoggo, this would work for EntryRow (as it is now) but yeh, not for everything...

@sandreae
Copy link
Member Author

I think what we had before was a little different from what I'm suggesting here actually.

Here's an example in the sqlx codebase: https://github.com/launchbadge/sqlx/blob/0b2844bf39128132aeccce2cfc02ec94472e074f/sqlx-core/src/decode.rs#L9-L59

I was thinking, for example, our EntryRow struct could contain proper p2panda-rs which get encoded as / decoded from strings in the background. Am I making sense here 🤣 ?

@adzialocha
Copy link
Member

I'm not sure if I understand correctly both your idea and the sqlx documentation, but I wonder if its not mixing up two different concepts?

EntryRow is a row of a table, meaning it contains multiple columns of different types. So implementing EntryRow as a type does not make much sense?

@adzialocha
Copy link
Member

adzialocha commented Apr 23, 2022

For writing traits for p2panda types it could just be that you can't implement traits for external structs. So even if you want to make it typed, you could only do it by creating a wrapper type first, ala: DoggoLogId(LogId), etc.

That would be fine with me though, on the other hand, if the database layer is clearly separated from the rest of the application, I also would not mind just handling String types and making sure they never leave that layer.

@sandreae
Copy link
Member Author

EntryRow is a row of a table, meaning it contains multiple columns of different types. So implementing EntryRow as a type does not make much sense?

Yeh, I had mixed this up! ;-p

In any case, I had a little go at a custom implementation of Decode (see below).

use std::error::Error;

use serde::{Deserialize, Serialize};
use sqlx::database::{Database, HasValueRef};
use sqlx::decode::Decode;
use sqlx::types::Type;

use p2panda_rs::identity::Author;

#[derive(Debug, Deserialize, Serialize)]
pub struct DoggoAuthor(Author);

impl<DB: Database> Type<DB> for DoggoAuthor {
    fn type_info() -> DB::TypeInfo {
        todo!()
    }
}

impl std::str::FromStr for DoggoAuthor {
    type Err = sqlx::error::Error;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        let author = Author::new(s).unwrap(); // don't unwrap here irl
        Ok(DoggoAuthor(author))
    }
}

// DB is the database driver
// `'r` is the lifetime of the `Row` being decoded
impl<'r, DB: Database> Decode<'r, DB> for DoggoAuthor
where
    // we want to delegate some of the work to string decoding so let's make sure strings
    // are supported by the database
    &'r str: Decode<'r, DB>,
{
    fn decode(
        value: <DB as HasValueRef<'r>>::ValueRef,
    ) -> Result<DoggoAuthor, Box<dyn Error + 'static + Send + Sync>> {
        // the interface of ValueRef is largely unstable at the moment
        // so this is not directly implementable

        // however, you can delegate to a type that matches the format of the type you want
        // to decode (such as a UTF-8 string)

        let value = <&str as Decode<DB>>::decode(value)?;

        // now you can parse this into your type (assuming there is a `FromStr`)

        Ok(value.parse()?)
    }
}

Don't really feel this is the right way to go, but was an interesting excursion.

As you say, if we have the db layer quite separate then maybe handling the strings is ok.

@sandreae
Copy link
Member Author

I actually had a go at expanding this into a working POC: https://github.com/p2panda/aquadoggo/tree/db-decode-experiment

See what you think, I quite like it as the strings are now parsed into (wrapped) p2panda types as they come out of the DB which is cool. Then when implementing the StorageProvider traits on top of this, we wouldn't have the ugly string parsing and unwrapping.

I don't see much difference between the two approaches now though tbh... it would be more whichever is best for code readability I think.

@adzialocha
Copy link
Member

Yeah, this looks also fine! The difference is maybe that there is some nice tooling to convert the types implementing the sqlx traits, so there is less code to write when implementing some database-related methods.

I can imagine it's advantage will show the more code we have 👍

@sandreae
Copy link
Member Author

Yeh, I think I may keep it in the back pocket for now, and once we're deeper in the implementation of all our database methods we can look again.

If we stick to the "everything is a string" approach we will have quite a lot of structs full of string fields, which isn't great for comprehension. But we have quite nice string parsing for many of our types (which we could expand) so... again, not so bad.

@sandreae
Copy link
Member Author

sandreae commented May 2, 2022

Another thought on this. There are some types (SchemaId, DocumentViewId) which potentially have different string representations which could be used when they are stored in the db. Implementing these traits would clearly define which we intend to use. On the flip side, it might get in the way if we actually want to store them in different formats in different places.

@adzialocha adzialocha added this to the Database & Store Layer milestone May 10, 2022
@sandreae
Copy link
Member Author

I still think this might be a really nice patter, let's wait til all the stores are implemented though and asses what we need then.

@sandreae
Copy link
Member Author

It's a "nice to have but not necessary" for sure.

@sandreae sandreae mentioned this issue May 19, 2022
9 tasks
@sandreae
Copy link
Member Author

sandreae commented Jun 9, 2022

We don't need this for now, always an option later if we have the need though.

@sandreae sandreae closed this as completed Jun 9, 2022
@adzialocha adzialocha removed this from the Database & Store Layer milestone Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants