Proposal: make it clearer which persistence methods do callbacks/validations

rubyonrails-core
1

Currently many of the persistence methods all have different behaviours when it comes to whether setters are called, and validations and or callbacks are run, Knowing what happens requires deeper knowledge of rails than just reading the method name

This can be confusing for newcomers and even people very familiar with rails will need to double check which does what.

save(validate: false) => callbacks: yes, validations: no, only_save_mentioned_columns: N/A

save => callbacks: yes, validations: yes, touch: yes, only_save_mentioned_columns: N/A

save(touch: false) => callbacks: yes, validations: yes, touch: no, only_save_mentioned_columns: N/A

save(validate: false, touch: false) => callbacks: yes, validations: no, touch: no, only_save_mentioned_columns: N/A

update/update_attributes => callbacks: yes, validations: yes, setters: yes, touch: yes, only_save_mentioned_columns: no

update_columns/update_column => callbacks: no, validations: no, setters: no, touch: no, only_save_mentioned_columns: yes

update_attribute => callbacks: yes, validations: no, setters: yes, touch: yes, only_save_mentioned_columns: no

increment!/decrement!/toggle! => callbacks: no, validations: no, setters: no, touch: no, only_save_mentioned_columns: yes

increment!/decrement!(touch: true) => callbacks: no, validations: no, setters: no, touch: yes, only_save_mentioned_columns: yes

touch => callbacks: some, validations: no, setters: no, touch: yes, only_save_mentioned_columns: yes

update_all/Class update => callbacks: no, validations: no, setters: no, touch: no, only_save_mentioned_columns: yes

Proposal A:

Follow what the save pattern started, and provide explicit instructions to update, and a more complete list to save

  1. Add callbacks: false option to save.

  2. Add validate:,touch:,callbacks:,only_save_mentioned_columns: to update, with the option of providing the instructions in a separate hash for models where those are columns/attributes

e.g. update(column: whatever, callbacks: false), or update(**params, callbacks: false) or update({ column: whatever }, callbacks: false), or update(params, callbacks: false)

  1. deprecate the other update_* methods

  2. always use attributes/setters. if one wants to not use the setters, don’t use these pretty methods, and instead use =/write_attribute and then save

  3. only_save_mentioned_columns is a bad name, but I’m having difficulty thinking of a better one that’s as clear

  4. validate the keyword arguments passed to save/update, so people don’t accidentally write ‘skip_callbacks: true’ or ‘validates: false’ or etc

Proposal B:

Rather than modifying update_*/save signatures, deprecate and replace with a method called persist and persist! that takes those options

I’d be happy to undertake the work of doing this PR if this is something you’d be interested in merging

2

Currently many of the persistence methods all have different behaviours when it comes to whether setters are called, and validations and or callbacks are run, Knowing what happens requires deeper knowledge of rails than just reading the method name

This can be confusing for newcomers and even people very familiar with rails will need to double check which does what.

IMO most of these are clarified by clicking the “source” link under the method in the docs to see how they use the core primitives - i.e., `update` calls `save` internally.

save(validate: false) => callbacks: yes, validations: no, only_save_mentioned_columns: N/A save => callbacks: yes, validations: yes, touch: yes, only_save_mentioned_columns: N/A save(touch: false) => callbacks: yes, validations: yes, touch: no, only_save_mentioned_columns: N/A save(validate: false, touch: false) => callbacks: yes, validations: no, touch: no, only_save_mentioned_columns: N/A

I doubt this is anybody’s favorite interface, but it’s better than the old `save(false)`. AFAIK it’s partly an artifact of how `save` is implemented as a stack of included modules.

update/update_attributes => callbacks: yes, validations: yes, setters: yes, touch: yes, only_save_mentioned_columns: no update_attribute => callbacks: yes, validations: no, setters: yes, touch: yes, only_save_mentioned_columns: no

`update_attributes` is deprecated now on master, because it was confusing. Calling `update_attribute` on a record with unsaved changes is generally an antipattern, but its behavior is clearly documented.

update_columns/update_column => callbacks: no, validations: no, setters: no, touch: no, only_save_mentioned_columns: yes increment!/decrement!/toggle! => callbacks: no, validations: no, setters: no, touch: no, only_save_mentioned_columns: yes increment!/decrement!(touch: true) => callbacks: no, validations: no, setters: no, touch: yes, only_save_mentioned_columns: yes touch => callbacks: some, validations: no, setters: no, touch: yes, only_save_mentioned_columns: yes

These are special-purpose methods and don’t belong in this list, IMO.

update_all/Class update => callbacks: no, validations: no, setters: no, touch: no, only_save_mentioned_columns: yes

These last two aren’t the same - the former generates a single UPDATE query, but the latter loads each object and calls `update`. This may count as evidence for your point about “need[ing] to double check which does what” above. :slight_smile:

Proposal A:

Follow what the `save` pattern started, and provide explicit instructions to update, and a more complete list to save

1. Add `callbacks: false` option to save.

Which callbacks run when you specify `callbacks: false, validate: true`? In particular, does this skip `before_validation`/`after_validation`?

IMO if you’re skipping callbacks on a generic call to `save` you’ve got callbacks that might not really be callbacks.

2. Add `validate:`,`touch:`,`callbacks:`,`only_save_mentioned_columns:` to update, with the option of providing the instructions in a separate hash for models where those are columns/attributes e.g. `update(column: whatever, callbacks: false)`, or `update(**params, callbacks: false)` or `update({ column: whatever }, callbacks: false)`, or `update(params, callbacks: false)`

Not a fan of taking this many boolean flags.

6. validate the keyword arguments passed to save/update, so people don't accidentally write 'skip_callbacks: true' or 'validates: false' or etc

This is the best suggestion in this proposal IMO, but it’s hard to do since the flags are consumed by different parts of the `save` method implemented in different files.

Proposal B:

Rather than modifying update_*/save signatures, deprecate and replace with a method called `persist` and `persist!` that takes those options

IMO if Proposal B was the status quo, we’d be talking about giving frequently-used combinations of those boolean flags specific names like “update_column” and deprecating `persist`.

To sum up:

* the only things that currently skip callbacks are specialized methods explicitly intended to do very limited updates to the DB. IMO this shouldn’t change

* similarly for `only_save_mentioned_columns`; only the specialized methods skip saving dirty columns

* `validate` and `touch` are already boolean options to `save`. They aren’t available in `update`, but `update` is literally just “assign attributes and call save” so IMO it’s trivial to get a similar result if needed.

* in conclusion, I don’t see how exposing more of the internal implementation as controllable options *reduces* the need to understand the internals

—Matt Jones