Remove implicit filter in update method

[tqwewe]

TLDR;

Instead of using the Set primary key in an active model for updating an entity, use a manual .filter method.

Fruit::update(pear)
    .filter(fruit::Column::Id.eq(123)) // Explicitly filter for the ID only
    .exec(db).await?;

And add a new method directly to an ActiveModel which uses the Set primary key.

let pear = fruit::ActiveModel { id: Set(123), ... };
pear.update().exec(db).await?; // No need for `.filter`, it uses the ID already

---

Currently to perform an update on multiple fields, it looks like you do it with:

Fruit::update_many()
    .col_expr(fruit::Column::CakeId, Expr::value(Value::Null))
    .filter(fruit::Column::Name.contains("Apple"))
    .exec(db)
    .await?;

But if you want to update 10 columns, do you need to manually add .col_expr for each column?

And doing the following will update the pears name with the ID 123.

let pear = pear::ActiveModel { 
    id: Set(123),
    name: Set("Green Pear".to_string()),
};
Fruit::update(pear).exec(db).await?;

The issue with this, is that if the id is not set, a panic will occur.
https://github.com/SeaQL/sea-orm/blob/de1f5d397df26659ef11ee9cc97251964503c748/src/query/update.rs#L89-97

But perhaps it'd make sense for the API to remove the lines 89 - 97 (in the link above) and force the user to explicitly add the WHERE id = '...' clause:

Fruit::update(pear)
    .filter(fruit::Column::Id.eq(123))
    .exec(db).await?;

The benefit of this, is it allows users to write more complex filtering rather than a simple id = ....
The down side is that if the ActiveModel has the ID as Set... then it'll actually try to update the ID in the query. But I don't consider this so much as a down side, because some people may actually want to update a primary key.

The active model could have an .update method which could keep the current behaviour and use the set primary key to perform an update.
Eg:

let pear = pear::ActiveModel { ... };
pear.update(db).await?; // Update the pear in place using it's set ID.

[billy1624]

But if you want to update 10 columns, do you need to manually add .col_expr for each column?

Yes, this is the case.

The issue with this, is that if the id is not set, a panic will occur.

I think we better use Rust's type system to handle this instead of throwing panic during runtime, as discussed earlier.

The benefit of this, is it allows users to write more complex filtering rather than a simple id = ....
The down side is that if the ActiveModel has the ID as Set... then it'll actually try to update the ID in the query. But I don't consider this so much as a down side, because some people may actually want to update a primary key.

I think we can add a functionality to reset the filter.

Fruit::update(pear)
    .reset_filter()
    .filter(fruit::Column::Id.eq(123))
    .exec(db).await?;

The active model could have an .update method which could keep the current behaviour and use the set primary key to perform an update.

Agree!

[tyt2y3]

I agree on this behavior indeed.

[billy1624]

However, this will be a huge breaking change for those who are using Entity::update(active_model) directly. One must manually apply filter on their own. Otherwise, all rows will be updated.

---- query::update::tests::update_1 stdout ----
thread 'query::update::tests::update_1' panicked at 'assertion failed: `(left == right)`
  left: `"UPDATE \"cake\" SET \"name\" = 'Apple Pie'"`,
 right: `"UPDATE \"cake\" SET \"name\" = 'Apple Pie' WHERE \"cake\".\"id\" = 1"`', src/query/update.rs:196:9

[tqwewe]

I think it's great to make these kind of changes at this time as SeaORM was released so recently.. so I think these changes are worth it!

[tyt2y3]

sea-orm/src/query/update.rs

Lines 261 to 291 in f56ac7b

	#[test]
	fn update_5() {
	assert_eq!(
	Update::many(fruit::Entity)
	.set(fruit::ActiveModel {
	name: ActiveValue::set("Apple".to_owned()),
	cake_id: ActiveValue::set(Some(3)),
	..Default::default()
	})
	.filter(fruit::Column::Id.eq(2))
	.build(DbBackend::Postgres)
	.to_string(),
	r#"UPDATE "fruit" SET "name" = 'Apple', "cake_id" = 3 WHERE "fruit"."id" = 2"#,
	);
	}
	#[test]
	fn update_6() {
	assert_eq!(
	Update::many(fruit::Entity)
	.set(fruit::ActiveModel {
	id: ActiveValue::set(3),
	..Default::default()
	})
	.filter(fruit::Column::Id.eq(2))
	.build(DbBackend::Postgres)
	.to_string(),
	r#"UPDATE "fruit" SET "id" = 3 WHERE "fruit"."id" = 2"#,
	);
	}
	}

After a third thought, I came to a solution that we don't need to change existing behavior, while still does what you were asking for.

[tyt2y3]

Works for you? @acidic9

[tqwewe]

Although it's not my preferred final API, it does work. Would be lovely if other people contribute their opinions too

[tyt2y3]

Close for now. To whom may stumble upon, feel free to leave a comment.