#[changeset_for] now skips optional fields when None

My original thoughts on how to implement this involved boxing and
sticking in a `Vec`. Unfortunately, that requires implementing
`AsChangeset` for `UserChanges` and not `&UserChanges`, which I don't
want to do by default.

If you actually want to assign `NULL`, you can still do so by doing
`update(table).set(column.eq(None))`. In the future we might introduce
an additional type to make it possible to assign null through codegen.

With this change in implementation, I've removed the impl of `Changeset`
for `Vec<T>`, as it feels redundant with a tuple containing options.

I've done a best effort to make sure we generate valid SQL in the
various cases. There is still one error case, when the resulting
changeset has 0 fields to update. This will be corrected in another PR,
as I'm still considering whether we should do magic to return the same
thing as we would otherwise, or whether it's an error case.

Fixes #26

CHANGELOG.md

@@ -5,6 +5,12 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 
 ## Unreleased
 
+### Changed
+
+* `#[changeset_for(table)]` now treats `Option` fields as an optional update.
+  Previously a field with `None` for the value would insert `NULL` into the
+  database field. It now does not update the field if the value is `None`.
+
 ### Fixed
 
 * `#[derive(Queriable)]` now allows generic parameters on the struct.

diesel/src/expression/predicates.rs

@@ -149,6 +149,10 @@ impl<T, U> Changeset for Eq<T, U> where
 {
     type Target = T::Table;
 
+    fn is_noop(&self) -> bool {
+        false
+    }
+
     fn to_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
         try!(out.push_identifier(T::name()));
         out.push_sql(" = ");

diesel/src/query_builder/update_statement/changeset.rs

@@ -15,6 +15,7 @@ pub trait AsChangeset {
 pub trait Changeset {
     type Target: QuerySource;
 
+    fn is_noop(&self) -> bool;
     fn to_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult;
 }
 
@@ -28,24 +29,29 @@ impl<T> AsChangeset for T where
     }
 }
 
-impl<T: Changeset> Changeset for Vec<T> {
-    type Target = T::Target;
-
-    fn to_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
-        for (i, changeset) in self.iter().enumerate() {
-            if i != 0 {
-                out.push_sql(", ");
-            }
-            try!(changeset.to_sql(out))
-        }
-        Ok(())
-    }
-}
-
 impl<T: Changeset + ?Sized> Changeset for Box<T> {
     type Target = T::Target;
 
+    fn is_noop(&self) -> bool {
+        (&**self).is_noop()
+    }
+
     fn to_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
         (&**self).to_sql(out)
     }
 }
+
+impl<T: Changeset> Changeset for Option<T> {
+    type Target = T::Target;
+
+    fn is_noop(&self) -> bool {
+        self.is_none()
+    }
+
+    fn to_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
+        match self {
+            &Some(ref c) => c.to_sql(out),
+            &None => Ok(()),
+        }
+    }
+}

diesel/src/types/impls/tuples.rs

@@ -123,12 +123,21 @@ macro_rules! tuple_impls {
             {
                 type Target = Target;
 
+                fn is_noop(&self) -> bool {
+                    $(e!(self.$idx.is_noop()) &&)+ true
+                }
+
                 fn to_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
+                    let noop_element = true;
                     $(
-                        if e!($idx) != 0 {
-                            out.push_sql(", ");
+                        let needs_comma = !noop_element;
+                        let noop_element = e!(self.$idx.is_noop());
+                        if !noop_element {
+                            if needs_comma {
+                                out.push_sql(", ");
+                            }
+                            try!(e!(self.$idx.to_sql(out)));
                         }
-                        try!(e!(self.$idx.to_sql(out)));
                     )+
                     Ok(())
                 }

diesel_codegen/README.md

@@ -102,11 +102,16 @@ additional configurations.
 Adds an implementation of the [`AsChangeset`][as_changeset] trait to the
 annotated item, targeting the given table. At this time, it only supports
 structs with named fields. Tuple structs and enums are not supported. See [field
-annotations][#field-annotations] for additional configurations. If the struct
-has a field for the primary key, an additional function, `save_changes(&mut
-self, connection: &Connection) -> QueryResult<()>`, will be added to the model.
-This will persist the model to the database and update it with any fields the
-database returns.
+annotations][#field-annotations] for additional configurations.
+
+Any fields which are of the type `Option` will be skipped when their value is
+`None`. This makes it easy to support APIs where you may not want to update all
+of the fields of a record on every request.
+
+If the struct has a field for the primary key, an additional function,
+`save_changes(&mut self, connection: &Connection) -> QueryResult<()>`, will be
+added to the model.  This will persist the model to the database and update it
+with any fields the database returns.
 
 [queriable]: http://sgrif.github.io/diesel/diesel/query_source/trait.Queriable.html
 [insertable]: http://sgrif.github.io/diesel/diesel/trait.Insertable.html

diesel_codegen/src/update.rs

@@ -1,9 +1,9 @@
 use aster;
-use syntax::ast::{self, MetaItem};
+use syntax::ast::{self, MetaItem, TyPath};
 use syntax::codemap::Span;
 use syntax::ext::base::{Annotatable, ExtCtxt};
 use syntax::ptr::P;
-use syntax::parse::token::InternedString;
+use syntax::parse::token::{InternedString, intern_and_get_ident};
 
 use attr::Attr;
 use model::Model;
@@ -120,7 +120,19 @@ fn changeset_ty(
         .segment(table).build()
         .segment(attr.column_name).build()
         .build();
-    let field_ty = &attr.ty;
+    if let Some(ty) = ty_param_of_option(&attr.ty) {
+        let inner_ty = inner_changeset_ty(cx, column, &ty);
+        quote_ty!(cx, Option<$inner_ty>)
+    } else {
+        inner_changeset_ty(cx, column, &attr.ty)
+    }
+}
+
+fn inner_changeset_ty(
+    cx: &mut ExtCtxt,
+    column: ast::Path,
+    field_ty: &ast::Ty,
+) -> P<ast::Ty> {
     quote_ty!(cx,
         ::diesel::expression::predicates::Eq<
             $column,
@@ -143,5 +155,25 @@ fn changeset_expr(
         .segment(attr.column_name).build()
         .build();
     let field_name = &attr.field_name.unwrap();
-    quote_expr!(cx, $column.eq(&self.$field_name))
+    if is_option_ty(&attr.ty) {
+        quote_expr!(cx, self.$field_name.as_ref().map(|f| $column.eq(f)))
+    } else {
+        quote_expr!(cx, $column.eq(&self.$field_name))
+    }
+}
+
+fn ty_param_of_option(ty: &ast::Ty) -> Option<&P<ast::Ty>> {
+    match ty.node {
+        TyPath(_, ref path) => {
+            path.segments.first().iter()
+                .filter(|s| s.identifier.name.as_str() == intern_and_get_ident("Option"))
+                .flat_map(|s| s.parameters.types().first().map(|p| *p))
+                .next()
+        }
+        _ => None,
+    }
+}
+
+fn is_option_ty(ty: &ast::Ty) -> bool {
+    ty_param_of_option(ty).is_some()
 }

diesel_tests/tests/lib.in.rs

@@ -1,3 +1,4 @@
 mod schema;
 mod insert;
 mod deserialization;
+mod update;

diesel_tests/tests/lib.rs

@@ -24,4 +24,3 @@ mod select;
 mod transactions;
 mod types;
 mod types_roundtrip;
-mod update;

diesel_tests/tests/update.rs

@@ -125,3 +125,35 @@ fn save_on_struct_with_primary_key_changes_that_struct() {
 
     assert_eq!(user, user_in_db);
 }
+
+#[test]
+fn option_fields_on_structs_are_not_assigned() {
+    use schema::users::dsl::*;
+
+    let connection = connection_with_sean_and_tess_in_users_table();
+    update(users.filter(id.eq(1)))
+        .set(hair_color.eq("black"))
+        .execute(&connection).unwrap();
+    let mut user = User::new(1, "Jim");
+    user.save_changes(&connection).unwrap();
+
+    let expected_user = User::with_hair_color(1, "Jim", "black");
+    assert_eq!(expected_user, user);
+}
+
+#[test]
+fn sql_syntax_is_correct_when_option_field_comes_before_non_option() {
+    #[changeset_for(users)]
+    struct Changes {
+        hair_color: Option<String>,
+        name: String,
+    }
+
+    let changes = Changes { hair_color: None, name: "Jim".into() };
+    let connection = connection_with_sean_and_tess_in_users_table();
+    let user = update(users::table.filter(users::id.eq(1))).set(&changes)
+        .get_result(&connection);
+
+    let expected_user = User::new(1, "Jim");
+    assert_eq!(Ok(expected_user), user);
+}