What is the problem in the following piece of code?
private def render_create
ActiveRecord::Base.transaction do
if @my_model.save!
render json: @my_model, status: :created
else
render json: @my_model.errors, status: :unprocessable_entity
end
end
end
I will give you 3 seconds to discover it!
While you're thinking about the problem… What we are trying to do here is an UPSERT. It is basically an insert or update. We will have two renders and we will render different type of HTTP status code, depending if the action was creation or update.
The problem in our first piece of code: It will never enter in the else!
Rails has a lot of useful methods to be used with our models, but we need to take care to not write this kind of things. Because of that, we need to test it! One test will make sure we were always entering in the if method and never inside the else.
In our case, our code should look like:
private def render_create
if @my_model.save
render json: @my_model, status: :created
else
render json: @my_model.errors, status: :unprocessable_entity
end
end
Just that! Without the ActiveRecord::Transaction, because the we don't need to launch an exception, if my_model cannot be saved, it should return false and then it will enter in my else and render the errors and have a proper status code. BTW, you can find on the internet a good list of Rails status code to be used in our controller methods.
To understand this problem, you just needed to know the difference between save and save!:
save, by default, always run the validations. If any of them fail, the action is canceled and save returns false.
save!, by default, always run the validations. If any of them fail, ActiveRecord::RecordInvalid gets raised!
Similar problems
We can have the same kind of problem if we were going to use any other method with ! in a similar situation.
For example, in the following piece of code we have a similar problem.
private def render_update(params)
ActiveRecord::Base.transaction do
if @my_model.update_attributes!(params)
render json: @my_model, status: :ok
else
render json: @my_model.errors, status: :unprocessable_entity
end
end
end
In this case, the else will never be reached. If update_attributes! fails, it will launch an exception.
Using needless transactions in our code and methods we don't understand fully is bad practice.
Look at our main upsert method and try to understand what it does:
def upsert
ActiveRecord::Base.transaction do
@my_model = MyModel.find(my_model_params[:id])
if @my_model
render_update
else
render_create
end
end
end
What this method should do: create my_model if it does not exist, otherwise just update the method.
What this method is doing: If the model doesn't exist, it launches an exception because the find method launches an exception and it never creates a new record.
This is because we're using the method find. We also don't need the ActiveRecord::Base.transaction in here. Let's refactor this!
def upsert
@my_model = MyModel.find_or_initialize_by(id: my_model_params[:id])
if @my_model.persisted?
render_create
else
render_update
end
end
P.S.: This upsert definition is not very common in Rails applications. Normally what people do is just create a method to create and another for update. But for the sake of illustration, we are using the upsert methods rendering two different things in different methods.
We will use a method find_or_initialize_by rather than find. It finds the model if it exists, otherwise, it will just initialize using a new and not a create.
What you should learn with these three little examples:
- Do not use
ActiveRecord::Base.transaction in all the places you can see. Try to imagine if this is really necessary.
- Pay attention to the Active Record methods you're using. Depending on the situation, it can have a totally different behavior you have in mind.