Impl From/ToSql & Queryable for PathBuf/OsString

[AzHicham]

Hello,

Is it possible to implement FromSql, ToSql for PathBuf as is it the case for String and also Queryable, Insertable, Identifiable, AsChangeset ?

Right now I have some boilerplate code to do that, for PathBuf and Vec< PathBuf > :

Btw I know I could also use #[diesel(serialize_as = Vec<u8>, deserialize_as = Vec<u8>)] but still I need AsChangeset, AsExpression.

```rust

#[derive(Debug, Clone, AsExpression, PartialEq, PartialOrd, Serialize, Deserialize)]
#[diesel(sql_type = sql_types::Bytea)]
pub struct DbPath(pub PathBuf);

impl<DB> ToSql<sql_types::Bytea, DB> for DbPath
where
    DB: Backend,
    [u8]: ToSql<sql_types::Bytea, DB>,
{
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, DB>) -> serialize::Result {
        self.0.as_os_str().as_bytes().to_sql(out)
    }
}

impl<B: Backend> FromSql<sql_types::Bytea, B> for DbPath
where
    Vec<u8>: FromSql<sql_types::Bytea, B>,
{
    fn from_sql(bytes: RawValue<B>) -> deserialize::Result<Self> {
        <Vec<u8> as FromSql<sql_types::Bytea, B>>::from_sql(bytes)
            .map(|s| DbPath(OsStr::from_bytes(&s).into()))
    }
}

```

Thank you

[weiznich]

It's certainly possible to implement these traits in diesel, although it's not clear if that's really what we as maintainers want to do. Can you present your usecase why that would be required and also describe how these values would be represented at database level? The general guidelines for type mappings in diesel are as following:

1. It's a commonly used type (that's the case here)
2. FromSql/ToSql cannot fail due to values not representable by this type
3. It's possible to implement both: ToSql and FromSql for the same type

[AzHicham]

Hello,
First of all thanks for the quick reply.
My use case: My app need to store in a database (postgresql) filesystem paths (ie PathBuf and Vec of PathBuf).
I could choose to store String instead of PathBuf but I will miss non UTF-8 cases and still need in my app to handle all faillible conversion from PathBuf to String + Windows UTF-16.

1. Ok
2. The best type I found in postgresql to map PathBuf if BYTEA, Plus as you can see in my implementation the conversion cannot fail
3. both FromSql & ToSql can be implemented for PathBuf and OsString

best regards

[weiznich]

For OsString the point 2 is not true. OsString on windows are assumed to contain UTF-16 encoded strings, which is for obvious reasons not true for random byte sequences. So mapping from Bytea to OsString may fail. PathBuf is described as thin wrapper around OsString so the same argument seems to be true there as well. Given that problem I would rather not accept such an impl in diesel.

Btw I know I could also use #[diesel(serialize_as = Vec, deserialize_as = Vec)] but still I need AsChangeset,

I would happily accept a PR that adds #[diesel(serialize_as)] support for AsChangeset as well.

[AzHicham]

Hello,

Sorry for the late reply.
My bad you're right, I suppose we cannot add this only for Unix ? :p

I would happily accept a PR that adds #[diesel(serialize_as)] support for AsChangeset as well.

I did not really understand what you mean here ?
Should I add support for #[diesel(serialize_as = Vec< u8 >)] or for #[diesel(serialize_as)] globally ?

Well In fact after more test #[diesel(serialize_as = Vec< u8 >)] is not enough because when I add Queryable & Insertable it doesn't work anymore :(

[weiznich]

I did not really understand what you mean here ?
Should I add support for #[diesel(serialize_as = Vec< u8 >)] or for #[diesel(serialize_as)] globally ?

Currently the #[diesel(serialize_as)] attribute is not supported by #[derive(AsChangeset)] as you've noticed above. What I wanted to say with that comment: I would accept a PR that adds support for that attribute to #[derive(AsChangeset)], not that it already works.

[AzHicham]

About possible failure :
I'm not sure it can fail on Windows
As stated by the doc :
Note, OsString and OsStr internally do not necessarily hold strings in the form native to the platform; While on Unix, strings are stored as a sequence of 8-bit values, on Windows, where strings are 16-bit value based as just discussed, strings are also actually stored as a sequence of 8-bit values, encoded in a less-strict variant of UTF-8. This is useful to understand when handling capacity and length values.

So the function OsStr::from_bytes never panic or fail
And you can find some example in the doc :

#[cfg(unix)] {
    use std::ffi::OsStr;
    use std::os::unix::ffi::OsStrExt;

    // Here, the values 0x66 and 0x6f correspond to 'f' and 'o'
    // respectively. The value 0x80 is a lone continuation byte, invalid
    // in a UTF-8 sequence.
    let source = [0x66, 0x6f, 0x80, 0x6f];
    let os_str = OsStr::from_bytes(&source[..]);

    assert_eq!(os_str.to_string_lossy(), "fo�o");
}
#[cfg(windows)] {
    use std::ffi::OsString;
    use std::os::windows::prelude::*;

    // Here the values 0x0066 and 0x006f correspond to 'f' and 'o'
    // respectively. The value 0xD800 is a lone surrogate half, invalid
    // in a UTF-16 sequence.
    let source = [0x0066, 0x006f, 0xD800, 0x006f];
    let os_string = OsString::from_wide(&source[..]);
    let os_str = os_string.as_os_str();

    assert_eq!(os_str.to_string_lossy(), "fo�o");
}

[weiznich]

I need to think about that. It might be an argument for allowing these mappings, on the other hand it doesn't seem like a natural representation of file paths at database level.
@diesel-rs/contributors Do you have any opinion here?

[AzHicham]

Nice, If you have a better type in mind I'll take it.
FYI this my database (with pathbuf <-> ByteA) in pycharm bytea column are automatically display as Text.

[weiznich]

Given the argumentation above and the fact that operators like like also work with byte strings I would be in favor of accepting a PR that adds these mappings if it includes documentation + tests.

[AzHicham]

Cool I'll try to do that soon :)
Thank you :)

[AzHicham]

Hello :)
I start working on it, it may takes some time ^^ my branch here
Right now my code works under unix, I still need to implement the windows case.

About test & documentation where should I add tests, diesel is pretty big and I kind of lost ?

Thank you

[weiznich]

It's always fine to ask for help where to add stuff or anything other that might be unclear while implementing features.
Here and here are good places for tests. For the documentation you would just extend the list of supported mappings on the corresponding sql type. So just add the newly added types there.

That written, it's also sometimes helpful to just open an in-progress PR as that makes it much easier to discuss specific code changes there.

From looking at the relevant change: It might be useful to add some information about cross-platform compatibility to the documentation as well. Also we need to document that this mapping is only there if the platform is unix or windows (I think there are platforms where both cfg's are set to false).