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 support for Uuid to SQLite #364

Closed
flosse opened this Issue Jul 1, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@flosse

flosse commented Jul 1, 2016

It would be nice to be able to use uuid::Uuid within the structs in combination of SQLite!

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 1, 2016

As far as I'm aware there is no data type in SQLite that maps to UUID?

@flosse

This comment has been minimized.

flosse commented Jul 1, 2016

As far as I'm aware there is no data type in SQLite that maps to UUID?

I'm sorry. You're totally right!

@flosse flosse closed this Jul 1, 2016

@theduke

This comment has been minimized.

Contributor

theduke commented Apr 9, 2017

No there is not, but the UUID could just be (de)serialized to BINARY or TEXT, which would be much nicer than having to convert manually. @sgrif

@0xcaff

This comment has been minimized.

0xcaff commented May 23, 2018

@sameer

This comment has been minimized.

sameer commented Aug 15, 2018

I'm interested @0xcaff, would appreciate it if you have the time to do a PR for this.

@0xcaff

This comment has been minimized.

0xcaff commented Aug 15, 2018

I’d like thoughts from a contributor before making a PR. Thoughts @sgrif?

@weiznich

This comment has been minimized.

Contributor

weiznich commented Aug 15, 2018

Our policy for including mappings between certain SQL types an rust types is like following:

  • Either a rust type is able to represent all possible values of the matched SQL type and also the SQL type could represent all values of the rust type
  • Or the type is a fundamental type like boolean.
    The first condition is not fulfilled because there at Text and also Binary values that are no valid uuid's (for example all shorter or longer values)
    In my opinion also the second point is not fulfilled because uuid is not that fundamental like for example booleans. (If anyone from @diesel/core disagrees, pleas response here 😉)

Given that (and given that it is quite easy to write a new type wrapper for this outside of diesel) I would say that impl should not be part of diesel itself.

@0xcaff

This comment has been minimized.

0xcaff commented Aug 15, 2018

Playing devils advocate here.

I'd argue that uuid is a fundamental type. Many applications use the uuid create and diesel already has support for using it with some SQL dialects (postgres). It's useful to have a built, tested and discoverable implementation for users of diesel. Writing a new type wrapper is annoying because of the need for boilerplate type conversion code.

Also, chrono::NaiveDate violates these rules. It is serialized as TEXT for SQLite.

/// The date SQL type.
///
/// ### [`ToSql`](../serialize/trait.ToSql.html) impls
///
/// - [`chrono::NaiveDate`][NaiveDate] with `feature = "chrono"`
///
/// ### [`FromSql`](../deserialize/trait.FromSql.html) impls
///
/// - [`chrono::NaiveDate`][NaiveDate] with `feature = "chrono"`
///
/// [NaiveDate]: /chrono/naive/date/struct.NaiveDate.html
#[derive(Debug, Clone, Copy, Default, QueryId, SqlType)]
#[postgres(oid = "1082", array_oid = "1182")]
#[sqlite_type = "Text"]
#[mysql_type = "Date"]
pub struct Date;

@sameer

This comment has been minimized.

sameer commented Aug 16, 2018

Either it should be implemented for both or none. It's misleading to implement it for NaiveDate and not Uuid since both can technically be serialized.

If it shouldn't be, is there some way this could be provided in a standardized manner? It is odd for users to have to manually implement ToSql and FromSql for types that can be serialized in general.

@weiznich

This comment has been minimized.

Contributor

weiznich commented Aug 16, 2018

In my opinion NaiveDate (and similar types) falls in the same category like booleans. They are fundamental in such a way that it is not possible to write larger application without using them. So not supporting the is not an option.
On the other hand it is possible to write large application without using uuid's, because for nearly all use cases using a 64 bit identifier should be enough.

@sgrif

This comment has been minimized.

Member

sgrif commented Aug 16, 2018

We also support it because SQLite does have a ton of built-in functions for dealing with dates. Yes, it represents them as strings, but that's ultimately an implementation detail, not something relevant to whether the type exists or not. Importantly, there is a canonical way to represent a date in SQLite.

That is not the true of UUIDs. We could store the raw bytes, or we could store the text representation. It's not obvious which we should choose, and we would be incompatible with existing databases when we choose one or the other.

Ultimately when there's a clear representation of a type and semantically it exists for a given backend, we support it even if there's some mismatch. For example, even PG's datetime type supports a different range of dates than chrono. Given that SQLite is fully dynamically typed, if we really wanted to only support what strictly could be represented, the only type that we could support for SQLite is Vec<u8>. We do have to draw the line somewhere though, and this is where we've chosen to draw it for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment