Pay Attention to your Active Record methods

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.