save_changes() doesn't save the field when it's None, but should save it (as null)

[Boscop]

save_changes() doesn't save the field when it's None, only when it's Some(val).

That's very counter-intuitive. It should save all the fields!
So when the field is None, it should save it (as null in sql).

It should behave differently than an Option field in a normal changeset struct (used with normal update queries), because a normal struct has different expected semantics (and has to have them, to signify not changing a field).

BUT: With save_changes() you expect ALL changes to be saved. And when you don't want to change a field, you just don't modify the struct before calling save_changes().

[sgrif]

I don't think save_changes() should behave any differently than updates everywhere else. You can configure this behavior with #[changeset_options(treat_none_as_null = "true")] on your struct.

[Boscop]

Thanks, #[changeset_options(treat_none_as_null = "true")] seems like an acceptable workaround for now, but do you see my point?:

• The expected semantics of save_changes() after setting a field to None are like saving a text file in an editor, after setting one line to empty (not removing the line). It would still save this change!
• The semantic difference between save_changes() and update() is the difference between overwriting a file after changing some lines and applying a patch to a file.
• Usually, if one wants to be able to call both save_changes() and update() on a table, they would define a separate changeset struct for update() that only includes the fields they are "patching".

[sgrif]

I do see your point. However, I think that having updates always work consistently, and keeping save_changes easy to understand in terms of every other form of update is more valuable. I had actually considered deprecating save_changes for a while, since it's pointless on PG and is at a weird level of abstraction compared to the rest of the library.

I disagree with your assertion that save_changes and update should require separate structs. Keep in mind that AsChangeset in general is meant to be on structs which are one-to-one with your input source (e.g. they would also have #[derive(Deserialize)]). Keeping that use case in mind, there's not a great way to differentiate from "this was not included in the input" vs "this was explicitly assigned to null" other than Some(None). Of course depending on your use case, that may not be what you want, hence why we have the option.

I recommend reading through #26 for more context.

[Boscop]

Thanks for the quick reply.

By input you mean the recommended use-case in an API server is to deserialize the request directly into a changeset struct?

I never do that, I always have separate data types for controllers (different requests act on data in different ways and not every request should be allowed to change all fields, e.g. editing a reply post should not allow changing its parent_id), models (the ORM structs) and views (different users can see different fields).
So for edit requests I always deserialize into a FooEdit struct first, do the validation on that, then create a FooUpdate struct instance from that and then call update() with that. (My controllers never call diesel functions directly, they call methods on the ORM struct that call the diesel functions.)
So I usually define a FooUpdate struct for every Foo struct that participates in the API.
But for internal ORM structs that are not changed by requests directly, I usually derive AsChangeset on Foo itself and then call save_changes() directly on a Foo instance.

• How common is it to deserialize a request directly into the actual changeset struct?

• And in those use cases, wouldn't that changeset struct usually be a separate struct (FooUpdate) than the model struct (Foo), with the fields suitably Option-ized? (So it would still allow Foo::save_changes() to behave differently than updating with a FooUpdate.) Otherwise, the API would be very insecure, every model field could be changed by a request (unless the controller does extra measures to prevent that, like using record update syntax with only overwriting the allowed fields, but it's too easy to forget doing that.)

[sgrif]

So for edit requests I always deserialize into a FooEdit struct first, do the validation on that, then create a FooUpdate struct instance from that and then call update() with that.

This all sounds reasonable for validations at the moment. I suspect we will eventually end up with something that lives at a level close to serde that handles validations, where deserializing something that fails validation is considered a deserialization error.

How common is it to deserialize a request directly into the actual changeset struct?

I think we need more applications built before we can answer that. The intent is for them to live very close together though even if they aren't the same struct (ideally they should be able to though). This is one of the main reasons that Queryable is typically a separate struct. The point about "did this mean assign null or skip this field" still stands regardless of whether you have an intermediate struct or not.

And in those use cases, wouldn't that changeset struct usually be a separate struct (FooUpdate) than the model struct (Foo)

I'm assuming by "model struct" you mean the thing that implements Queryable, and yes they are typically different. Actually reading the rest of that paragraph, yeah I better see where you're coming from (you expect this to be a thing on Queryable structs which you're seeing as more traditional "models" in the Python/Ruby ORM sense). TBH if anything that just reinforces my desire to deprecate the function more than anything else, but it also is a situation where #[treat_none_as_null] handles your use case very well.

[Boscop]

Hm, shouldn't model structs be both Queryable and Identifiable? Should it be separate structs? But how would their fields differ?

I never used any other ORMs before diesel, I just learned how to use diesel about a year ago by looking at this project.. (But I knew some sql/postgres before.)
All its model structs are Queryable and Identifiable, (e.g. User here).
So in my projects I used the same structure, e.g.:

#[derive(Debug, Queryable, Identifiable, Associations, AsChangeset)]
#[belongs_to(Place)]
#[belongs_to(Customer)]
#[table_name = "clients"]
pub struct Client {
	pub id: i64,
	pub customer_id: Option<i64>,
	pub created_at: NaiveDateTime,
	pub updated_at: NaiveDateTime,
	pub place_id: Option<i64>,
	pub unique_id: String,
	pub version: i32,
	pub last_seen: NaiveDateTime,
}

#[derive(Insertable)]
#[table_name = "clients"]
pub struct NewClient<'a> {
	pub unique_id: &'a str,
	pub version: i32,
}

(And then I call e.g. client.save_changes().)

Btw, please don't deprecate save_changes(), it's very useful, and the above workaround makes it work in my use case :)

[sgrif]

Hm, shouldn't model structs be both Queryable and Identifiable?

Identifiable means "this struct has an identifier linking it to a single row in a database table. It is completely separate from Queryable which says "I can be deserialized from a SQL query", though Identifiable structs usually implement Queryable.

All its model structs are Queryable and Identifiable

Yup, and if all your structs are one-to-one with database tables, that's what it'll look like. We decouple those concepts by design. Same with AsChangeset. It definitely can be on the same struct as Queryable, but in my experience, the needs of your input for updates changes for different reasons than the structures you return on reads.

In Rust specifically for example, if you are building a web app, the id does not come from the same place as the rest of your input (it comes from the URL not the request body). I intend most usage to look like update(posts.find(params.get("id"))).set(&post_form). Similarly with inserts, the parent ID usually comes from the session or auth token, not the form input. So I would expect most inserts to look like insert_into(posts).values((user_id.eq(session.get("user_id")), &user_form)).

You don't have to follow these patterns, but they are separated for a reason.

Btw, please don't deprecate save_changes()

I'm not currently planning on it.