diesel-rs / diesel

A safe, extensible ORM and Query Builder for Rust
https://diesel.rs
Apache License 2.0
12.82k stars 1.08k forks source link

Allowing a type that's Queryable and Insertable to autoincrement its ID #1440

Closed brandur closed 6 years ago

brandur commented 6 years ago

Setup

Versions

Problem Description

I'm running into a problem whereby I'm not sure how to make a type both Queryable and Insertable while still having it autoincrement its primary key on inserts.

I have a Postgres table with a BIGSERIAL type like so:

CREATE TABLE episodes (
    id BIGSERIAL PRIMARY KEY,
    description TEXT,
...

In my generated schema, this translates to something like this as you'd expect:

table! {
    episodes (id) {
        id -> Int8,
        description -> Nullable<Text>,
...

And finally, I have a model in my code that I've marked as Insertable and Queryable:

#[derive(Insertable, Queryable)]
#[table_name = "episodes"]
pub struct Episode {
    pub id:           i64,
    pub description:  Option<String>,
...

I'm trying to figure out how I'd use this setup to insert a new record while delegating ID generation to the database. The SQL for this type of operation looks roughly like this:

INSERT INTO episodes (description, ...) 
    VALUES ('foo', ...);

Or like this:

INSERT INTO episodes (id, description, ...) 
    VALUES (DEFAULT, 'foo', ...);

If my id field is a simple i64, there's no valid value that will make the database autoincrement — 0, -1 etc. are all insertable BIGINTs and are inserted literally. As far as I know, only omitting the field or using the special DEFAULT value will do.

I tried changing my id to an Option<i64>:

pub struct Episode {
    pub id:           Option<i64>, // <-- changed from i64
    pub description:  Option<String>,
...

But then constraints on Queryable rightly kick in and tell me I'm not allowed to do this:

error[E0277]: the trait bound `std::option::Option<i64>: diesel::Queryable<diesel::types::BigInt, diesel::pg::Pg>` is not satisfied
   --> src/graphql.rs:155:14
    |
155 |             .load::<model::Episode>(&*context.get_conn()?)
    |              ^^^^ the trait `diesel::Queryable<diesel::types::BigInt, diesel::pg::Pg>` is not implemented for `std::option::Option<i64>`
    |
    = help: the following implementations were found:
              <std::option::Option<T> as diesel::Queryable<diesel::types::Nullable<ST>, DB>>
    = note: required because of the requirements on the impl of `diesel::Queryable<(diesel::types::BigInt, diesel::types::Nullable<diesel::types::Text>, diesel::types::Nullable<diesel::types::Bool>, diesel::types::Text, diesel::types::Nullable<diesel::types::Text>, diesel::types::Nullable<diesel::types::Text>, diesel::types::Text, diesel::types::BigInt, diesel::types::Timestamptz, diesel::types::Text), diesel::pg::Pg>` for `(std::option::Option<i64>, std::option::Option<std::string::String>, std::option::Option<bool>, std::string::String, std::option::Option<std::string::String>, std::option::Option<std::string::String>, std::string::String, i64, chrono::DateTime<chrono::Utc>, std::string::String)`
    = note: required because of the requirements on the impl of `diesel::Queryable<(diesel::types::BigInt, diesel::types::Nullable<diesel::types::Text>, diesel::types::Nullable<diesel::types::Bool>, diesel::types::Text, diesel::types::Nullable<diesel::types::Text>, diesel::types::Nullable<diesel::types::Text>, diesel::types::Text, diesel::types::BigInt, diesel::types::Timestamptz, diesel::types::Text), diesel::pg::Pg>` for `model::Episode`
    = note: required because of the requirements on the impl of `diesel::query_dsl::LoadQuery<_, model::Episode>` for `diesel::query_builder::SelectStatement<schema::episodes::table, diesel::query_builder::select_clause::DefaultSelectClause, diesel::query_builder::distinct_clause::NoDistinctClause, diesel::query_builder::where_clause::WhereClause<diesel::expression::operators::Eq<schema::episodes::columns::podcast_id, diesel::expression::bound::Bound<diesel::types::BigInt, i64>>>, diesel::query_builder::order_clause::OrderClause<diesel::dsl::Desc<schema::episodes::columns::published_at>>, diesel::query_builder::limit_clause::LimitClause<diesel::expression::bound::Bound<diesel::types::BigInt, i64>>>`

Any advice? Thanks!

(This looks a little like #640, but I don't think it's the same.)

brandur commented 6 years ago

Ah, I just found this section on Insertable in a draft guide.

When implementing Insertable, you probably won't be setting the auto-incremented id field of the row. Usually you will also ignore fields such as created_at and updated_at. For this reason, it's not advisable to use Queryable and Insertable on the same struct due to the field number constraints of Queryable. Create another struct that you may use for database insertions that will have all the fields you would like to set. This section will not cover nullable fields (we'll cover that in AsChangeset), so we will assume every field must have data in our example. When making a separate struct for database inserts, Diesel needs to know the corresponding table name, so the struct must also be annotated with the #[table_name="some_table_name"] attribute. If your new struct has different field names, each of them may be annotated with #[column_name(some_column_name)].

I'll do this for now, but this is a little suboptimal given the amount of overlap you're going to have between the insertable and queryable versions for most models — models tend to balloon to lots and lots of fields given enough time, and all of them will need to be written twice (although I suppose the approach has some advantages as well).

sgrif commented 6 years ago

given the amount of overlap you're going to have between the insertable and queryable versions for most models

This is what I like to call "incidental duplication" -- which is the term I use to refer to things that happen to be structurally similar right now, but change in the future for different reasons. The approach we've taken is very specifically designed to separate things that represent the results of queries from things that represent input from external sources. The desire to keep these separate comes from experience with hundreds of applications. You can see similar approaches in other modern frameworks like Phoenix.

If you really want to share these structs, Queryable composes with other structs that are Queryable. You could have a struct like this:

struct UserWithId {
    id: i32,
    user: User,
}

It sounds like you've found the answers you're looking for, so I'm going to close this issue.

anilanar commented 6 years ago

@sgrif What do you mean by:

If you really want to share these structs, Queryable composes with other structs that are Queryable.

When I have the following:

#[derive(Queryable)]
struct UserWithId {
    id: i32,
    user: User,
}

#[derive(Queryable)]
struct User {
    age: i32,
}

users::table.load::<UserWithId>(&conn)?; emits an error:

error[E0277]: the trait bound `(i32, User): diesel::Queryable<(diesel::sql_types::Integer, diesel::sql_types::Integer), _>` is not satisfied
weiznich commented 6 years ago

@anilanar You need to add an explicit select clause to your query to transform the return type into the right one.

users::table.select((users::id, (users::age,))).load::<UserWithId>(&conn);

Should work. A other solution would be to manually implement Queryable

anilanar commented 6 years ago

It does work, but then it doesn't generate BelongsToDsl implementation for XWithId struct when foreign key field is defined in X struct. You can work around it by defining it in both XWithId and X and then do .select((x::id, x::user_id, (x::user_id, x::others)). Duplication again. Far from ideal 😕.

cisco-bth commented 2 years ago

Hello if anyone is interested in a different solution about this issue, I was just able to apply one which doesn't require a XWithID entity, I've resolved by having a ID primary key of type BYNARY(36), setting the id property of the entity struct as Vec type and then inserting the entity with the value of the id as

        id: Vec::from(Uuid::new_v4().to_string().as_bytes()),