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

Error deriving Insertable for a SQLite table with a nullable timestamp column #1242

Closed
nmusatti opened this Issue Oct 7, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@nmusatti

nmusatti commented Oct 7, 2017

Setup

Versions

  • Rust: 1.20.0 (stable)
  • Diesel: 0.16.0
  • Database: SQLite 3.7.17
  • Operating System CentOS 7.4

Feature Flags

  • diesel: sqlite chrono
  • diesel_codegen: sqlite

Problem Description

What are you trying to accomplish?

I have a table defined as follows:

create table books (
    id integer not null primary key,
    title varchar null,
    save_date timestamp null 
)

I'm trying to derive an Insertable implementation for it following roughly the Getting Started guide, using infer_schema! and the following struct:

	#[derive(Insertable)]
	#[table_name="books"]
	pub struct NewBook<'a> {
	    pub title : Option<&'a str>,
	    pub save_date : Option<&'a chrono::NaiveDateTime>, 
	}

What is the expected output?

A successful compilation, as is the case with diesel 0.14

What is the actual output?

A series of error messages complaining for unsatisfied trait bounds, such as these:

error[E0277]: the trait bound `chrono::NaiveDateTime: diesel::Expression` is not satisfied
  --> src/main.rs:30:11
   |
30 |    #[derive(Insertable)]
   |             ^^^^^^^^^^ the trait `diesel::Expression` is not implemented for `chrono::NaiveDateTime`
   |
   = note: required because of the requirements on the impl of `diesel::Expression` for `&'a chrono::NaiveDateTime`
   = note: this error originates in a macro outside of the current crate

error[E0277]: the trait bound `&'insert domain::NewBook<'a>: diesel::Insertable<schema::__diesel_infer_schema::infer_books::books::table, DB>` is not satisfied
  --> src/main.rs:30:11
   |
30 |    #[derive(Insertable)]
   |             ^^^^^^^^^^ the trait `diesel::Insertable<schema::__diesel_infer_schema::infer_books::books::table, DB>` is not implemented for `&'insert domain::NewBook<'a>`
   |
   = help: the following implementations were found:
             <&'insert domain::NewBook<'a> as diesel::Insertable<schema::__diesel_infer_schema::infer_books::books::table, DB>>
   = note: required by `diesel::Insertable`
   = note: this error originates in a macro outside of the current crate

Are you seeing any additional errors?

The full list of errors can be found in this gist.

Steps to reproduce

This project exemplifies the problem; uncomment the sections that are commented out and build. If you change the diesel and diesel_codegen version to 0.14 and dotenv's to 0.9 you'll see that it compiles and runs without problems.

Checklist

  • I have already looked over the issue tracker for similar issues.
@sgrif

This comment has been minimized.

Member

sgrif commented Oct 8, 2017

This appears to have been caused by 38038f6. It occurs for all reference types except str and slices. It's happening because after that commit we are attempting to use a value of type &'a &'b Value, which does not implement AsExpression.

We haven't implemented that in the past, as there's really no reason to take a reference other than for str and slice, since virtually every other type we support is Copy. That said, we should probably just allow double references here in general. I really wish we could do a blanket impl for this

sgrif added a commit that referenced this issue Oct 8, 2017

Always implement `AsExpression` for double references
The only time that a double reference `AsExpression` impl comes up is
from the derived impl of `Insertable` and `AsChangeset`. We can't vary
the generated code based on whether the type of a field is `Copy`, so we
have to assume that it isn't and always take a reference.

Previously we only did this for `str` and slices, as `String` and `Vec`
are the only non-copy types that we support (I think `BigDecimal` may
fall into that category now, but it's unlikely that avoiding a single
clone of that type is ever relevant).

Still, there's no reason that we shouldn't allow references of any type
to appear in `Insertable` structs. To avoid code bloat, I've cleaned
things up a bit and implemented more stuff on `str` and `[T]` directly,
letting the blanket reference impl apply to those types when it's
relevant.

Fixes #1242.
@sgrif

This comment has been minimized.

Member

sgrif commented Oct 8, 2017

I've opened a PR to fix this on master, but I won't be backporting it to 0.16 as there's very little reason to use &NaiveDateTime here instead of just NaiveDateTime.

sgrif added a commit that referenced this issue Oct 9, 2017

Always implement `AsExpression` for double references
The only time that a double reference `AsExpression` impl comes up is
from the derived impl of `Insertable` and `AsChangeset`. We can't vary
the generated code based on whether the type of a field is `Copy`, so we
have to assume that it isn't and always take a reference.

Previously we only did this for `str` and slices, as `String` and `Vec`
are the only non-copy types that we support (I think `BigDecimal` may
fall into that category now, but it's unlikely that avoiding a single
clone of that type is ever relevant).

Still, there's no reason that we shouldn't allow references of any type
to appear in `Insertable` structs. To avoid code bloat, I've cleaned
things up a bit and implemented more stuff on `str` and `[T]` directly,
letting the blanket reference impl apply to those types when it's
relevant.

Fixes #1242.

sgrif added a commit that referenced this issue Oct 9, 2017

Always implement `AsExpression` for double references
The only time that a double reference `AsExpression` impl comes up is
from the derived impl of `Insertable` and `AsChangeset`. We can't vary
the generated code based on whether the type of a field is `Copy`, so we
have to assume that it isn't and always take a reference.

Previously we only did this for `str` and slices, as `String` and `Vec`
are the only non-copy types that we support (I think `BigDecimal` may
fall into that category now, but it's unlikely that avoiding a single
clone of that type is ever relevant).

Still, there's no reason that we shouldn't allow references of any type
to appear in `Insertable` structs. To avoid code bloat, I've cleaned
things up a bit and implemented more stuff on `str` and `[T]` directly,
letting the blanket reference impl apply to those types when it's
relevant.

Fixes #1242.

@sgrif sgrif closed this in #1243 Oct 10, 2017

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