Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RF-DOCS] Active Record Callbacks #51654

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

Ridhwana
Copy link
Contributor

Motivation / Background

This Pull Request has been created to provide more clarity and update the Active Record Callbacks documentation.

Detail

This guide has probably been reviewed multiple times over time, so we can do a quick pass to see where we can make things clearer, simpler, and/or more concise.

Some initial thoughts to get started:

  • Samples that use Proc.new {} can likely be simplified to just use proc {}
  • Link to the validations guide where we mention it under conditional callbacks
  • after_find callbacks are also triggered by a few other methods: take, sole, find_by!
  • We could potentially add some more examples on different types of callbacks that aren't there.
  • Updated some sections for clarity

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Ridhwana and others added 30 commits March 28, 2024 13:16
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Ridhwana and others added 10 commits May 2, 2024 12:33
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: hatsu <hajiwata0308@gmail.com>
Co-authored-by: hatsu <hajiwata0308@gmail.com>
Co-authored-by: hatsu <hajiwata0308@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
@Ridhwana
Copy link
Contributor Author

Ridhwana commented May 2, 2024

Very nice updates overall @Ridhwana , the guide flows much more naturally. I had a few bits of feedback/suggestions below.

Thanks, @carlosantoniodasilva, your feedback has been great as usual! I've updated the guides based on your feedback, and would appreciate if you could double-check these:

Copy link
Contributor

@brunoprietog brunoprietog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great update!

guides/source/active_record_callbacks.md Show resolved Hide resolved
guides/source/active_record_callbacks.md Show resolved Hide resolved
guides/source/active_record_callbacks.md Outdated Show resolved Hide resolved
guides/source/active_record_callbacks.md Outdated Show resolved Hide resolved
@Ridhwana Ridhwana requested a review from p8 May 24, 2024 08:08
end
```

```irb
irb> @baby = Baby.create
Congratulations!
irb> cake = BirthdayCake.create
Copy link
Member

@p8 p8 May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny icing-on-the-cake, we can remove assigning the variable. Sorry, for the pun 🍰

Suggested change
irb> cake = BirthdayCake.create
irb> BirthdayCake.create

Comment on lines +46 to +47
As you will see, there are many life cycle events and you can choose to hook
into any of these either before, after, or even around them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reading this in the preview, this sentence was a bit difficult to parse.

Maybe split it up into two sentences?

Suggested change
As you will see, there are many life cycle events and you can choose to hook
into any of these either before, after, or even around them.
As you will see, there are many life cycle events. You can also hook into any of
these either before, after, or even around them.

Or phrase the options similar to "many life cycle events" ?

Suggested change
As you will see, there are many life cycle events and you can choose to hook
into any of these either before, after, or even around them.
As you will see, there are many life cycle events and multiple options to hook
into these, either before, after, or even around them.

Or with an em dash?

Suggested change
As you will see, there are many life cycle events and you can choose to hook
into any of these either before, after, or even around them.
As you will see, there are many life cycle events and multiple options to hook
into these either before, after, or even around them.

NOTE: If an `ActiveRecord::RecordNotDestroyed` is raised within `after_destroy`, `before_destroy` or `around_destroy` callback, it will not be re-raised and the `destroy` method will return `false`.
NOTE: If an `ActiveRecord::RecordNotDestroyed` is raised within `after_destroy`,
`before_destroy` or `around_destroy` callback, it will not be re-raised and the
`destroy` method will return `false`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can expand the examples, and move some of the content out of the warnings?

The whole callback chain is wrapped in a transaction. If any callback raises an
exception, the execution chain gets halted, a **rollback** is issued, and the
error will be re-raised.

```ruby
class Product < ActiveRecord::Base
   before_validation do
      raise "Price can't be negative" if total_price < 0
   end
end
Product.create # raises "Price can't be negative" 
```

This unexpectedly breaks code that does not expect methods like `create` and `save` to raise exceptions.
Instead, use `throw :abort` to intentionally halt the chain. If any callback throws `:abort`, the process will be aborted and `create` will return false.

```ruby
class Product < ActiveRecord::Base
   before_validation do
      throw :abort if total_price < 0
   end
end
Product.create # => false
```

WARNING: Any exception that is not ActiveRecord::Rollback, ActiveRecord::RecordNotSaved or 
ActiveRecord::RecordInvalid will be re-raised by Rails after the callback chain is halted.

When `throw :abort` is called in destroy callbacks, `destroy` will return false:

```ruby
class User < ActiveRecord::Base
   before_destroy do
      throw :abort if still_active?
   end
end
User.first.destroy # => false
```

It raises a `ActiveRecord::RecordNotDestroyed` when calling `destroy!`.
```ruby
User.first.destroy! # => raises an ActiveRecord::RecordNotDestroyed
```

Looking at some of the test cases, it seems throw :abort raises an ActiveRecord::RecordNotSaved if called in a before_* callback (so that is another error we should also mention).


WARNING. Avoid calls to `update`, `save` or other methods which create side-effects to the object inside your callback. For example, don't call `update(attribute: "value")` within a callback. This can alter the state of the model and may result in unexpected side effects during commit. Instead, you can safely assign values directly (for example, `self.attribute = "value"`) in `before_create` / `before_update` or earlier callbacks.
WARNING. Refrain from using methods like `update`, `save`, or any other methods
that cause side effects on the object within your callback functions. <br><br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nitpick, this is the only place we use callback functions.
Maybe use methods, although that adds a third methods to the sentence:

Suggested change
that cause side effects on the object within your callback functions. <br><br>
that cause side effects on the object within your callback methods. <br><br>

Or just keep the original callbacks?

Suggested change
that cause side effects on the object within your callback functions. <br><br>
that cause side effects on the object within your callbacks. <br><br>

Comment on lines +57 to +58
You can implement the callbacks as a **macro-style method that calls an ordinary
method** for registration.
Copy link
Member

@p8 p8 May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordinary method is the implementation, the macro-style method is used for registration.
The sentence what somewhat confusing to me. Maybe:

Suggested change
You can implement the callbacks as a **macro-style method that calls an ordinary
method** for registration.
You can register the callbacks with a **macro-style class method that calls an ordinary
method** for implementation.


irb> @user.save # updating @user
irb> user.save # updating @user
User was saved to database
```

### `after_save_commit`
Copy link
Member

@p8 p8 May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should after_save_commit be added to the list of after_commit aliases? Or maybe this header level should be decreased to make it fall under the "after_commit aliases" header?

Suggested change
### `after_save_commit`
#### `after_save_commit`


This order can be set via configuration:
If for some reason, you'd still like them to run in reverse you can set the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this sentence flow better if we move the comma?

Suggested change
If for some reason, you'd still like them to run in reverse you can set the
If for some reason you'd still like them to run in reverse, you can set the

See also: https://english.stackexchange.com/questions/504268/if-for-any-reason-comma#504270

reused by other models. Active Record makes it possible to create classes that
encapsulate the callback methods, so they can be reused.

Here's an example where we create a class with an `after_commit` callback to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is pretty long.
Maybe we can shorten it?

Suggested change
Here's an example where we create a class with an `after_commit` callback to
Here's an example of an `after_commit` callback class to

Comment on lines +1136 to +1149
WARNING. When a transaction completes, the `after_commit` or `after_rollback`
callbacks are called for all models created, updated, or destroyed within that
transaction. However, if an exception is raised within one of these callbacks,
the exception will bubble up and any remaining `after_commit` or
`after_rollback` methods will _not_ be executed. As such, if your callback code
could raise an exception, you'll need to rescue it and handle it within the
callback in order to allow other callbacks to run. <br><br> `after_commit` makes
very different guarantees than `after_save`, `after_update`, and
`after_destroy`. For example, if an exception occurs in an `after_save` the
transaction will be rolled back and the data will not be persisted. However,
during `after_commit` the data was already persisted to the database, and thus
any exception won't roll anything back anymore. Also note that the code executed
within `after_commit` or `after_rollback` callbacks is itself not enclosed
within a transaction. <br><br> In the context of a single transaction, if you
Copy link
Member

@p8 p8 May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning is pretty hard to read in the preview. The original three seperate warnings wasn't ideal either.
I think this is an important section that might deserve more detail?

Maybe we can only wrap the last sentences as a WARNING and add more examples?

Suggested change
WARNING. When a transaction completes, the `after_commit` or `after_rollback`
callbacks are called for all models created, updated, or destroyed within that
transaction. However, if an exception is raised within one of these callbacks,
the exception will bubble up and any remaining `after_commit` or
`after_rollback` methods will _not_ be executed. As such, if your callback code
could raise an exception, you'll need to rescue it and handle it within the
callback in order to allow other callbacks to run. <br><br> `after_commit` makes
very different guarantees than `after_save`, `after_update`, and
`after_destroy`. For example, if an exception occurs in an `after_save` the
transaction will be rolled back and the data will not be persisted. However,
during `after_commit` the data was already persisted to the database, and thus
any exception won't roll anything back anymore. Also note that the code executed
within `after_commit` or `after_rollback` callbacks is itself not enclosed
within a transaction. <br><br> In the context of a single transaction, if you
When a transaction completes, the `after_commit` or `after_rollback`
callbacks are called for all models created, updated, or destroyed within that
transaction. However, if an exception is raised within one of these callbacks,
the exception will bubble up and any remaining `after_commit` or
`after_rollback` methods will _not_ be executed.
```ruby
class User < ActiveRecord::Base
after_commit { raise }
after_commit { # this won't get called }
end
```
WARNING. As such, if your callback code
could raise an exception, you'll need to rescue it and handle it within the
callback in order to allow other callbacks to run.
`after_commit` makes
very different guarantees than `after_save`, `after_update`, and
`after_destroy`. For example, if an exception occurs in an `after_save` the
transaction will be rolled back and the data will not be persisted.
```ruby
class User < ActiveRecord::Base
after_save do
# If this fails the user won't be saved.
EventLog.create!(event: "user_saved")
end
end
```
However, during `after_commit` the data was already persisted to the database, and thus
any exception won't roll anything back anymore.
```ruby
class User < ActiveRecord::Base
after_commit do
# If this fails the user was already saved.
EventLog.create!(event: "user_saved")
end
end
```
WARNING. Also note that the code executed
within `after_commit` or `after_rollback` callbacks is itself not enclosed
within a transaction.
In the context of a single transaction, if you

Comment on lines +1136 to +1160
WARNING. When a transaction completes, the `after_commit` or `after_rollback`
callbacks are called for all models created, updated, or destroyed within that
transaction. However, if an exception is raised within one of these callbacks,
the exception will bubble up and any remaining `after_commit` or
`after_rollback` methods will _not_ be executed. As such, if your callback code
could raise an exception, you'll need to rescue it and handle it within the
callback in order to allow other callbacks to run. <br><br> `after_commit` makes
very different guarantees than `after_save`, `after_update`, and
`after_destroy`. For example, if an exception occurs in an `after_save` the
transaction will be rolled back and the data will not be persisted. However,
during `after_commit` the data was already persisted to the database, and thus
any exception won't roll anything back anymore. Also note that the code executed
within `after_commit` or `after_rollback` callbacks is itself not enclosed
within a transaction. <br><br> In the context of a single transaction, if you
interact with multiple loaded objects that represent the same record in the
database, there's a crucial behavior in the `after_commit` and `after_rollback`
callbacks to note. These callbacks are triggered only for the first object of
the specific record that changes within the transaction. Other loaded objects,
despite representing the same database record, will not have their respective
`after_commit` or `after_rollback` callbacks triggered. This nuanced behavior is
particularly impactful in scenarios where you expect independent callback
execution for each object associated with the same database record. It can
influence the flow and predictability of callback sequences, leading to potential
inconsistencies in application logic following the transaction.
influence the flow and predictability of callback sequences, leading to
potential inconsistencies in application logic following the
transaction.<br><br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WARNING. When a transaction completes, the `after_commit` or `after_rollback`
callbacks are called for all models created, updated, or destroyed within that
transaction. However, if an exception is raised within one of these callbacks,
the exception will bubble up and any remaining `after_commit` or
`after_rollback` methods will _not_ be executed. As such, if your callback code
could raise an exception, you'll need to rescue it and handle it within the
callback in order to allow other callbacks to run. <br><br> `after_commit` makes
very different guarantees than `after_save`, `after_update`, and
`after_destroy`. For example, if an exception occurs in an `after_save` the
transaction will be rolled back and the data will not be persisted. However,
during `after_commit` the data was already persisted to the database, and thus
any exception won't roll anything back anymore. Also note that the code executed
within `after_commit` or `after_rollback` callbacks is itself not enclosed
within a transaction. <br><br> In the context of a single transaction, if you
interact with multiple loaded objects that represent the same record in the
database, there's a crucial behavior in the `after_commit` and `after_rollback`
callbacks to note. These callbacks are triggered only for the first object of
the specific record that changes within the transaction. Other loaded objects,
despite representing the same database record, will not have their respective
`after_commit` or `after_rollback` callbacks triggered. This nuanced behavior is
particularly impactful in scenarios where you expect independent callback
execution for each object associated with the same database record. It can
influence the flow and predictability of callback sequences, leading to potential
inconsistencies in application logic following the transaction.
influence the flow and predictability of callback sequences, leading to
potential inconsistencies in application logic following the
transaction.<br><br>
interact with multiple loaded objects that represent the same record in the
database, there's a crucial behavior in the `after_commit` and `after_rollback`
callbacks to note. These callbacks are triggered only for the first object of
the specific record that changes within the transaction. Other loaded objects,
despite representing the same database record, will not have their respective
`after_commit` or `after_rollback` callbacks triggered.
```ruby
class User < ApplicationRecord
after_commit :log_user_saved_to_db, on: :update
private
def log_user_saved_to_db
Rails.logger.info("User was saved to database")
end
end
```
```ruby
irb> user = User.create
irb> User.transaction { user.save; user.save }
# User was saved to database
```
WARNING: This nuanced behavior is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs rails foundation Rails Foundation PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants