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

Is there a way to use strong types like `UserId`? #1014

Closed
vityafx opened this Issue Jul 10, 2017 · 26 comments

Comments

Projects
None yet
6 participants
@vityafx

vityafx commented Jul 10, 2017

For example, I have a table users with id INTEGER PRIMARY KEY. Is there a way to use pub struct UserId(pub i32) instead of i32 for the user id?

@killercup

This comment has been minimized.

Member

killercup commented Jul 10, 2017

I don't think this is easily possible right now—you'd need to implement a bunch of traits, like ToSql and FromSql, cf. this test.

It'd be possible to write derive macro that makes common newtype case (for wrappers of primitive sql types) a bit more elegant by generating the implementations for you.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 10, 2017

I think that would be a strong candidate for something that should best live as a third party crate, not as part of Diesel itself. However, I'm happy to assist someone in implementing such a crate.

@killercup

This comment has been minimized.

Member

killercup commented Jul 10, 2017

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 10, 2017

It shouldn't need to. The impls should all be able to look something like:

pub struct UserId(pub i32);

impl<ST, DB> ToSql<ST, DB> for UserId
where
    i32: ToSql<ST, DB>,
{
    fn to_sql(...) {
        self.0.to_sql(...)
    }
}
@Eijebong

This comment has been minimized.

Member

Eijebong commented Jul 10, 2017

I think you'd also need the HasSqlType trait which is a bit more difficult as it's different for every backend.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 10, 2017

@Eijebong This isn't implementing a new SQL type. However, even if it were it could still be trivially delegated.

pub struct MyIntegerType;

impl<DB> HasSqlType<MyIntegerType> for DB
where
    DB: TypeMetadata + HasSqlType<Integer>,
{
    fn metadata() -> DB::TypeMetadata {
        <DB as HasSqlType<Integer>>::metadata()
    }
}
@Eijebong

This comment has been minimized.

Member

Eijebong commented Jul 10, 2017

Oh, right, this is the same trait for every backend... I'm dumb :)

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jul 13, 2017

I've just implemented a DieselNewType custom derive which seems to work, although I'm not sure what stress-testing it would look like: https://github.com/quodlibetor/diesel-newtype

I would appreciate some eyes on it (bug reports/pull requests). If the name seems reasonable and we can get it into good enough shape I'm happy to upload it to crates.io and/or move it into the diesel-rs org.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 13, 2017

I've only been using it for about an hour, and before this I hadn't actually tried sticking my newtypes into the DB via Diesel, so there are probably unknown unknowns

Lol I'd probably start with trying that out. ;)

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 13, 2017

Doesn't try to handle generics at all. I haven't encountered generics on newtypes so I didn't bother. They should be easy to add

This seems reasonable to me.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 13, 2017

It seems almost... too... easy...

That's the goal. ;)

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 13, 2017

I've only given it a cursory look, but it looks fine at first glance. You're missing a lot of impls that are required to use the newtype in every context you could use String. Particularly, you'll need AsExpression impls for every combination of nullable/not and reference/not, along with FromSqlRow for nullable/not. You'll want a Queryable impl as well so they can query for the newtype directly. These are the impls that we have for our primitive types

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jul 13, 2017

I've been wanting to create newtypes, but I've been too lazy to actually do the various From/Tos, so I just created a "ToUnderlying" trait that only my db modules have access to, and have been using that.

With this I've swapped out a couple of my IDs in a couple of structs and, like I said, it seems to do... oh you've responded.

Those all look easy enough, thanks! I'm heading out for the weekend soon, but I should be able to implement them early next week if no one beats me to it.

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jul 13, 2017

I couldn't find any obvious way of verifying that my derive generates something reasonable, is there an example test file that guarantees that everything works without a db, or should I just bite the bullet and copy your db setup code?

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 13, 2017

You'll want to verify against an actual database. Anything else will only be testing your assumptions.

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jul 13, 2017

And here I've been assuming my assumptions were great. That makes my decision making easier.

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 13, 2017

Could we have it as an example in the docs and tests?

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jul 13, 2017

Have what as an example in which docs/tests?

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 13, 2017

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jul 13, 2017

Ah, yeah a "custom types" guide, explaining what each trait enables as you implement it, would be nice to have. I might be able to write something like that as I add tests and discover additional functionality for diesel-newtype.

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 14, 2017

Exactly

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jul 29, 2017

I've implemented (thanks entirely to @sgrif's help) auto-queryable and auto-asexpression for diesel-newtype in quodlibetor/diesel-derive-newtype#2. There are even tests that verify that obvious things that should work do work. If anybody wants to try substituting in a newtype using it now might be a reasonable time, although admittedly there's a decent chance that what you'd be doing is helping me debug rather than actually making your code better.

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 5, 2017

So I ran into the fact that diesel's query builder operates pretty much entirely on the SQL types of AsExpression types, which means that the current implementation of NewTypes aren't type-safe all the way through to the query-builder. Documented here in this commit.

I haven't been able to come up with something that would allow strong typing on the rust side of the query builder and still... work... with the rest of diesel. It seems like it is by design that queries operate on SQL types. I mean, it makes sense too, it's not like it's a bad decision.

I'm starting work on a guide around implementing columnar types, and I'm curious if that is an accurate description of the way that diesel treats rust types once they enter the query builder. Additionally, that would affect how I document diesel-newtype.

@sgrif

This comment has been minimized.

Member

sgrif commented Aug 5, 2017

You'd need to create a custom domain type in PG to do what you're describing.

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 5, 2017

Okay so that sounds like my understanding is correct but databases may support doing it on their side. I didn't even consider that.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 16, 2017

Closing as there's nothing actionable here. See #595 (comment) for more thoughts, and #162 for the tracking issue on ergonomics here.

@sgrif sgrif closed this Dec 16, 2017

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