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

Insertable should allow Option on non null fields with default #554

Closed
mackeyja92 opened this Issue Dec 27, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@mackeyja92

mackeyja92 commented Dec 27, 2016

The Insertable trait currently does not allow fields that are specified as NOT NULL in the database but with a DEFAULT to have Option defined in the model.

For example, this model here would fail when using a postgres backend even though the database will automatically fill in the field.

#[table_name = "todos"]
#[derive(Queryable, Insertable)]
pub struct Todo {
    id: Option<i32>,
    pub description: String,
}

With the SQL definition of this

CREATE TABLE todos (
  id SERIAL PRIMARY KEY,
  description VARCHAR
);

The above SQL generates the following schema with the field being NOT NULL and having a DEFAULT.

                                                     Table "public.todos"
   Column    |       Type        |                     Modifiers                      | Storage  | Stats target | Description
-------------+-------------------+----------------------------------------------------+----------+--------------+-------------
 id          | integer           | not null default nextval('todos_id_seq'::regclass) | plain    |              |
 description | character varying |                                                    | extended |              |
Indexes:
    "todos_pkey" PRIMARY KEY, btree (id)

This should work not only for PKs but also any field that has a default in the schema.

@killercup

This comment has been minimized.

Member

killercup commented Dec 27, 2016

We talked on Gitter about this: logs.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 3, 2017

Hm... I feel like this used to work in the past. It definitely is meant to work (with or without a default being present, which we currently don't/can't verify)

sgrif added a commit that referenced this issue Jan 4, 2017

Allow inserting `Option<T>` into columns where the type is not nullable
This is important to allow structs with types like `Option<i32>` for
`id` to be used with `Insertable`, since the column has a default.
Ideally we would only allow this to work with columns that have a
default value. I may make that change in future, but at the moment we do
not have a way of knowing which columns have a default.

I am OK with the additional incorrect queries that this allows, as they
were already possible. There's no reason that `struct NewUser { name:
Option<String>, hair_color: Option<String>, }` shouldn't work if `struct
NewUser { hair_color: Option<String> }` works, as they both generate
incorrect queries (name is not null and has no default in this example).

I've documented the change as only affecting columns with a default, as
we reserve the right to disallow using `Option<T>` with columns that
have no default value in the future. Ideally before 1.0 I'd like to
replace this with a solution that doesn't leak the
`IntoNullable::SqlType` constraint in the public API.

Fixes #554.

@sgrif sgrif closed this in #567 Jan 4, 2017

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