Clarification on update

[mcasper]

So far update is the hardest to wrap my head around, and I was hoping for some clarification/guidance.

In the context of an HTTP API, with a users table that looks like:

table! {
    users {
        id -> Serial,
        first_name -> VarChar,
        last_name -> VarChar,
        email -> VarChar,
    }
}

how would you go about implementing a Users#update action?

My first instinct is to have something like:

#[changeset_for(users)]
pub struct UserChanges {
    id: i32,
    first_name: Option<String>,
    last_name: Option<String>,
    email: Option<String>,
}

fn update(req: SomeHTTPReq) -> SomeHTTPRes {
  let changeset = UserChanges {
    first_name: req.body.get("first_name"),
    // etc
  }

  changeset.save_changes(&connection)
}

Such that you end up updating whatever valid params the user sends, but that requires having Nullable columns in the table macro, when the columns aren't actually nullable.

Keep up all the great work!

[sgrif]

Yeah, you wouldn't want to make the columns Nullable, as it'll just end up trying to insert NULL into the column. Right now there's no good way to handle a field that you want to optionally update with a struct. I actually can't think of an easy way to do this with plain assignments, either. This is a valid use case though. I might be able to do something with the type system to have it know to skip the update if the column is not nullable, but this hole would still exist if you wanted to optionally update a nullable column.

This popped into my head as a possible solution. Thoughts?

#[changeset_for(users)]
pub struct UserChanges {
    id: i32,
    #[optional_update]
    first_name: Option<String>,
    #[optional_update]
    last_name: Option<String>,
    #[optional_update]
    email: Option<String>,
}

Could also specify it at the struct level.

#[changeset_for(users, behavior_when_none="skip")]
pub struct UserChanges {
    // ...
}

In the short term, I would just do 3 queries to work around this.

[sgrif]

So I think this is what the impl needs to look like regardless of codegen. I think I'll need to add some impls for Box<Changeset> to make this work, but it's already object safe so that should be fine.

impl AsChangeset<users> for UserChanges {
    type Changeset = Vec<Box<Changeset<Target=users>>>;

    fn as_changeset(self) -> Changeset {
        let changes = Vec::new();
        // push any fields we always update
        if let Some(first_name) = self.first_name {
            changes.push(Box::new(users::first_name.eq(first_name)))
        }
        // repeat for each field
        changes
    }
}

I don't think there's a way to do this without boxing, and also using a Vec or the code will just get horrendous. I think it's fine to write impl<U: Changeset<Target=T>, T> Changeset for Vec<T>, and impl<U: Changeset<Target=T>, T> Changeset for Box<T>

[sgrif]

I've pushed up the required impls. Can you try manually implementing AsChangeset on your struct and tell me if that works? If so we just need to make an annotation to standardize it.

[mcasper]

Trying to build

pub struct UserChanges {
    id: i32,
    first_name: Option<String>,
    last_name: Option<String>,
    email: Option<String>,
}

use self::users::dsl::*;

impl AsChangeset for UserChanges {
    type Changeset = Vec<Box<Changeset<Target=users::table>>>;

    fn as_changeset(self) -> Changeset<Target=users::table> {
        let changes = Vec::new();

        if let Some(first_name) = self.first_name {
            changes.push(Box::new(first_name.eq(first_name)))
        }

        changes
    }
}

table! {
    users {
        id -> Serial,
        first_name -> VarChar,
        last_name -> VarChar,
        email -> VarChar,
    }
}

produces the error:

   Compiling yaqb_test v0.1.0 (file:///Users/mattcasper/code/home/loudly/yaqb_test)
src/main.rs:25:1: 37:2 error: the trait `core::marker::Sized` is not implemented for the type `yaqb::query_builder::update_statement::changeset::Changeset<Target=users::table> + 'static` [E0277]
src/main.rs:25 impl AsChangeset for UserChanges {
src/main.rs:26     type Changeset = Vec<Box<Changeset<Target=users::table>>>;
src/main.rs:27
src/main.rs:28     fn as_changeset(self) -> Changeset<Target=users::table> {
src/main.rs:29         let changes = Vec::new();
src/main.rs:30
               ...
src/main.rs:25:1: 37:2 help: run `rustc --explain E0277` to see a detailed explanation
src/main.rs:25:1: 37:2 note: `yaqb::query_builder::update_statement::changeset::Changeset<Target=users::table> + 'static` does not have a constant size known at compile-time
src/main.rs:25:1: 37:2 note: required by `yaqb::query_builder::update_statement::changeset::AsChangeset`
error: aborting due to previous error

[mcasper]

And trying to use impl AsChangeset<users::table> for UserChanges { as shown above produces

 Compiling yaqb_test v0.1.0 (file:///Users/mattcasper/code/home/loudly/yaqb_test)
src/main.rs:25:6: 25:31 error: wrong number of type arguments: expected 0, found 1 [E0244]
src/main.rs:25 impl AsChangeset<users::table> for UserChanges {
                    ^~~~~~~~~~~~~~~~~~~~~~~~~
src/main.rs:25:6: 25:31 help: run `rustc --explain E0244` to see a detailed explanation
error: aborting due to previous error

[sgrif]

Your return type should be Self::Changeset from as_changeset

[sgrif]

And you'll need changes to be mutable

[mcasper]

The compiler is still sad for the same (core::marker::Sized is not implemented) reason :/

impl AsChangeset for UserChanges {
    type Changeset = Vec<Box<Changeset<Target=users::table>>>;

    fn as_changeset(self) -> Self::Changeset> {
        let mut changes = Vec::new();

        if let Some(first_name) = self.first_name {
            changes.push(Box::new(first_name.eq(first_name)))
        }

        changes
    }
}

[mcasper]

type Changeset = Vec<Box<Changeset<Target=users::table>>>; is the line it's complaining about, is users::table the right type to be setting as the Target?

[sgrif]

Whoops, I need to add some ?Sized to things. I will fix when I'm back at a computer.

[sgrif]

Just pushed an update. This compiles, and should work as a workaround (it's what I'll have the annotation generate)

impl AsChangeset for UserChanges {
    type Changeset = Vec<Box<Changeset<Target=users::table>>>;

    fn as_changeset(self) -> Self::Changeset {
        let mut changes: Vec<Box<Changeset<Target=users::table>>> = Vec::new();

        if let Some(first_name) = self.first_name {
            changes.push(Box::new(
                users::first_name.eq(first_name).as_changeset()
            ))
        }

        if let Some(last_name) = self.last_name {
            changes.push(Box::new(
                users::last_name.eq(last_name).as_changeset()
            ))
        }

        if let Some(email) = self.email {
            changes.push(Box::new(
                users::email.eq(email).as_changeset()
            ))
        }

        changes
    }
}

[mfpiccolo]

I was able to get update to work implementing AsChangeset, however I was only able to get it to build using execute_returning_count. I could not get it to return the result using query_one. This was the error that I was getting:

src/models/user.rs:54:18: 54:37 error: the trait `yaqb::expression::Expression` is not implemented for the type `yaqb::query_builder::update_statement::UpdateStatement<yaqb::query_source::filter::FilteredQuerySource<models::user::users::table, yaqb::expression::predicates::Eq<models::user::users::columns::id, yaqb::expression::bound::Bound<yaqb::types::Integer, i32>>>, collections::vec::Vec<Box<yaqb::query_builder::update_statement::changeset::Changeset<Target=models::user::users::table>>>>` [E0277]
src/models/user.rs:54     User::conn().query_one(&command)
                                       ^~~~~~~~~~~~~~~~~~~
src/models/user.rs:54:18: 54:37 help: run `rustc --explain E0277` to see a detailed explanation

and the same error with Query:

the trait `yaqb::query_builder::Query` is not implemented for the type ...

[mcasper]

@mfpiccolo I got update with AsChangeset to build just fine using query_one:

let changeset = UserChanges {
    id: user_id,
    first_name: body.get("first_name").map(|attr| attr[0].to_owned()),
    last_name: body.get("last_name").map(|attr| attr[0].to_owned()),
    email: body.get("email").map(|attr| attr[0].to_owned()),
};

let command = query_builder::update(users.filter(id.eq(changeset.id))).set(changeset);
let result: Option<User> = connection.query_one(command).unwrap();

[sgrif]

I think we're seeing rust-lang/rust#28894 strike again. The actual issue is that AsQuery is not implemented for &UpdateStatement<...>. It is implemented for UpdateStatement<...>. Hopefully #29 will make this less confusing.

[mfpiccolo]

Yep that was it. Passing the command itself and not the borrowed command was the issue.

[bwo]

This may be old news the people conversing here, but opaleye's approach to insert/update seems like a reasonably nice one, at least in haskell-land: https://github.com/tomjaguarpaw/haskell-opaleye/blob/master/Doc/Tutorial/TutorialManipulation.lhs#L48-L88

[sgrif]

@bwo Yeah, I'm going to basically give the ability to choose between the current behavior and that behavior. I don't want to stop supporting putting NULL in an update, though.

[sgrif]

I'm going to change the behavior of changeset_for to treat None as skip by default in 0.3. The more I think about it, the more clear it becomes that assigning null is not the majority case. You can still set null by manually doing set(column.eq(None)).

I will likely introduce an additional type in the future to represent wanting to assign null (which when deserializing from JSON will require the key being present, and the value being null)

[sgrif]

First pass at correcting this for those interested: #47

[Drakulix]

This is actually quite a major issue for my use case. I use my changeset struct as a proxy object for the actual database object, that is also cached and updates the records on Drop. I do not need fields, that get optionally updated, I am updating the whole record anyway. I need a way to clear the nullable fields.

Implementing AsChangeset manually is quite verbose, an additional option for the old behaviour would be very nice.

[sgrif]

FWIW it sounds like you don't actually need to use AsChangeset there, you can just call set((foo.eq(self.foo), bar.eq(self.bar)), etc directly, which might be slightly less verbose in your use case.

[Drakulix]

well that is still quite large and verbose, as my struct has a lot of fields. I will go with that for now, but it would be nice to be able to just use #[changeset_for].

[sgrif]

Yeah, I'll probably add an option for the behavior you want at some point. We just need to be careful to make sure the API is sufficiently clear what the difference is, and make sure its a wide enough use case to justify being in the core library, as we don't want to end up with 18-billion options (e.g. after this I wouldn't be surprised for someone to have a struct where they want one Option to have the new behavior, and another use the old, etc)

[Drakulix]

I totally understand that, but this recent change just moved the api hole to the less common use case, although there is a quite easy option to work around that.

Another Alternative might be to add a new enum that might either be Null or a value, that can be instanciated from an Option.
That would also solve the use case for an Option<Option<>> (if you would want to be able to optinally clear a value), without making the Api any more difficult for people not using the optional updates and not requiring anymore compiler extensions.

[sgrif]

Yeah, I've considered that in the past as well. There's a similar gap for inserts, where there's no way to insert NULL into a column which is nullable, but also has a default which is not NULL (a very niche case). My worry is that having another type to represent this would potentially just make actually using the struct painful. Then again, since the last time I really thought about that option, we've moved much towards having structs whose only purpose is to represent changes for the database, and not be used much in app code beyond that, so it might be worth investigating.

For just this case though, I think I'm fine with something like changeset_for(users, treat_none_as_null = "true").

[Drakulix]

Because I am not much a database guy I try to abstract away most of the database internals and just have a nice struct representation of my internal database structures. And that change would allow me easily to have one struct to rule all use cases without write much additional code.
So yes that change would be completely sufficient, thanks for looking into that.

[sgrif]

@Drakulix I've added the option in 59a25e8

[Drakulix]

@sgrif Thanks!