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.
save, by default, always run the validations. If any of them fail, the action is canceled and save returns
save!, by default, always run the validations. If any of them fail,
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
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
What you should learn with these three little examples:
- Do not use
ActiveRecord::Base.transactionin 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.