Recently I came across a bug that took me a day to understand what was happening. Below is a sample reproduction of the relevant files I was working with:
app/models/user.rb
class User < ApplicationRecord
# columns id, email, name
has_many :comments, dependent: :destroy
endapp/models/comment.rb
class Comment < ApplicationRecord
# columns id, text, user_id
belongs_to :user
after_validation :destroy_comment_if_empty
validates :user, presence: true
def destroy_comment_if_empty
destroy if text.blank?
end
endapp/services/upsert_user_service.rb
class UpsertUserService
include Virtus.model # it just allows us to declare instance attributes in a more handy way
attribute :comments_params
attribute :user_params
def call
User.transaction do
upsert_user
end
end
def upsert_user
user = User.find_or_initialize_by(email: user_params["email"])
user.assign_attributes(name: user_params["name"])
user.comments << comments
user.save!
end
def comments
comments_params["comments"].map { |comment_params| create_comment(comment_params) }
end
def create_comment(comment_params)
comment = user.comments.find_or_initialize_by(id: comment_params["id"])
comment.text = comment_params["text"])
comment
end
endUpsert means that if the record exists we update the record, if it doesn't we create the record.
The bug was happening at user.comments << comments in app/services/upsert_user_service.rb, in the method upsert_user:
class UpsertUserService
def upsert_user
user = User.find_or_initialize_by(email: user_params["email"])
user.assign_attributes(name: user_params["name"])
user.comments << comments # <--- FrozenError: can't modify frozen attributes
user.save!
end
endAfter debugging a little bit I got to a few conclusions:
- The bug only happened if the call to the service included a comment with a blank text.
- The bug only happened in the second call of the service (or in more accurate words, if the
useralready existed)
See if you can guess what is happening and then come back, because we are in for a ride.
To understand why it happened only in the second request we need to go back to the rails documentation about associations:
Adding an object to a collection (has_many or has_and_belongs_to_many) automatically saves that object, except if the parent object (the owner of the collection) is not yet stored in the database.
(source)
So when we did this the first time, user did not exist, which meant that this was only an assignment, no db access or whatsoever. It would be saved in the line below user.save!.
During the second request, user would already exist, so this line would trigger an automatic save. During this save one very interesting thing would happen: we have one callback after_validation that destroys the comment if it is blank.
So during the validation it would destroy the record (notice we start a destroy flow in the middle of a validation) and later when we attempted a save, this save with the record destroyed would throw the error: FrozenError: can't modify frozen attributes because destroying an object freezes it.
Mystery solved right? Closed case... Well mostly, for a fix to be developed this is enough information, but there were still way too many weird things that didn't make sense:
- We had a test that covered removing an existing
commentof an existinguser, why did that test pass? - If saving a
commentwithtext: ""failed, how did the first request withuser.save!worked?
So... we had a test to cover this case... or at least we thought. In the test case, we had an user and we had a comment with text and we updated the text of that comment to blank. And it passed! What? How is creating an empty comment different than updating one to be empty?
If you try to run the following snippet in isolation in the console you will see that it triggers a rollback
# setting up example
u = User.find(1)
u.comments = []
c1 = u.comments.create(text: "first")
c2 = u.comments.create(text: "second")
# executing
u = u.reload
c1 = u.comments.find_or_initialize_by(id: c1.id)
c1.text = ""
c2 = u.comments.find_or_initialize_by(id: c2.id)
c2.text = "SECOND"
u.comments << [c1, c2] # TRANSACTION (5.7ms) ROLLBACKThis last line is exactly what we are executing at user.comments << comments, so why doesn't the test break? Well it turns out that this concat operation opens a transaction as we can see here
And what happens with nested transactions? 💩 happens, basically a rollback of a nested transaction is ignored and is commited, you can find this documented here
(source)
One thing to note is: even if it wasn't a nested transaction this block would continue, the difference is that the rollback would've happened.
So the execution continues and if we check user.comments we will see that even though the save did not work, the attribution did! So saving it once again with user.save! works and succesfully destroys the comment with no text and makes our test pass.
Oh and why did it rollback during the update, but not during create? I believe it is because when we are creating we try to update id = newly_generated_id and this throws the Frozen error and when updating it makes a query for a record that doesn't exist triggering a rollback, but I didn't go after this, perhaps this could be for you reading? 😎
Well all that is nice and all, but one thing that bugged me the entire time was: how did calling save in a comment with no value did not work, but calling user.save! with the change in the comment work?
In order to understand that we need to once again do a deep dive in the rails codebase, it turns out that when rails is autosaving an association it doesn't do everything at once: it first validates it and then saves it, in two different operations. association.validate then association.save(validate: false).
Because of this quirk, we destroy the comment during the validation cycle and when we get to the line in the snippet above next if record.destroyed?, the record is already destroyed so it is skipped and not saved (which would break if we did just comment.save directly on a comment with blank text)
This brings us to the aftermath of everything, the solution isn't that interesting, we preemptively destroyed records with no value.
def comments
comments_params["comments"].filter_map do |comment_params|
comment = create_comment(comment_params)
next if commment.blank? && comment.destroy!
comment
end
endAlthough there were other solutions on the table that could be better:
Because we are building comments with user.comments.find_or_initialize_by(id: comment_params["id"]), we could stop attributing user.comments << comments and just save user.save! directly, but this has a problem documented here, where association attributes changed don't save properly. Unless we set has_many :comments, dependent: :destroy, autosave: true in user.rb, but this change could be risky as it affects all places where user is saved with comments.
We could create a association method and use it instead of doing an autosave.
class User < ApplicationRecord
has_many :comments, dependent: :destroy do
def find_inplace_or_initialize_by_id(id)
find do |comment|
comment.id == id
end || new()
end
end
endAnd using this method instead of user.comments.find_or_initialize_by, just beware that the method above is considerable slower if there are many comments.
PS.: One thing the documentation doesn't say is that:
post = Post.create(title: 'ruby rocks')
comment = post.comments.create(body: 'hello world')
comment.body = 'hi everyone'
post.save # => saves post, but not commentDoesn't save the comment, but
post = Post.create(title: 'ruby rocks')
post.comments.create(body: 'hello world')
comment = post.comments.first
comment.body = 'hi everyone'
post.save # => saves post and commentDoes save the post and comment