1

There is something weird happening with my Rails app. A controller action is called and saves a record to a table each time a user visits a unique url I created before.

Unfortunately, sometimes two identical records are created instead of only one. I added a "validates_uniqueness_of" but it's not working.

My controller code:

class ShorturlController < ApplicationController
  def show
    @shorturl = ShortUrl.find_by_token(params[:id])
    @card = Card.find(@shorturl.card_id)
    @subscriber = BotUser.find_by_sender_id(params['u'])
    @letter_campaign = Letter.find(@card.letter_id).campaign_name.downcase

    if AnalyticClic.where(card_id: @card.id, short_url_id: @shorturl.id, bot_user_id: @subscriber.id).length != 0
      @object = AnalyticClic.where(card_id: @card.id, short_url_id: @shorturl.id, bot_user_id: @subscriber.id)
      @ccount = @object[0].clicks_count
      @object.update(updated_at: Time.now, clicks_count: @ccount += 1)
    else
      AnalyticClic.create(card_id: @card.id, short_url_id: @shorturl.id, bot_user_id: @subscriber.id, clicks_count: "1".to_i)
    end

    @final_url = @card.cta_button_url

    redirect_to @final_url, :status => 301
  end
end

And the model:

class AnalyticClic < ApplicationRecord
  validates_uniqueness_of :bot_user_id, scope: :card_id
end

Any idea why sometimes I have duplicated records? The if should prevent that as well as the validates_uniqueness_of.

enter image description here

nico_lrx
  • 715
  • 1
  • 19
  • 36
  • you should put validation checks on the database level, not just on the models. See [does-rails-need-database-level-constraints](https://stackoverflow.com/questions/2589509/does-rails-need-database-level-constraints) – max pleaner Jul 24 '17 at 22:10

2 Answers2

2

First off, I believe your validation may need to look something like (although, TBH, your syntax may be fine):

class AnalyticClic < ApplicationRecord
  validates :bot_user_id, uniqueness: { scope: :card_id }
end

Then, I think you should clean up your controller a bit. Something like:

class ShorturlController < ApplicationController
  def show
    @shorturl = ShortUrl.find_by_token(params[:id])
    @card = Card.find(@shorturl.card_id)
    @subscriber = BotUser.find_by_sender_id(params['u'])
    @letter_campaign = Letter.find(@card.letter_id).campaign_name.downcase

    analytic_clic.increment!(:click_count, by = 1)

    @final_url = @card.cta_button_url

    redirect_to @final_url, :status => 301
  end

private

  def analytic_clic
    @analytic_clic ||= AnalyticClic.find_or_create_by(
      card_id: @card.id, 
      short_url_id: @shorturl.id, 
      bot_user_id: @subscriber.id
    )
  end

end 

A few IMPORTANT things to note:

You'll want to create an index that enforces uniqueness at the database level (as max pleaner says). I believe that'll look something like:

class AddIndexToAnalyticClic < ActiveRecord::Migration
  def change
    add_index :analytic_clics [:bot_user_id, :card_id], unique: true, name: :index_bot_user_card_id
  end
end

You'll want to create a migration that sets :click_count to a default value of 0 on create (otherwise, you'll have a nil problem, I suspect).

And, you'll want to think about concurrency with increment! (see the docs)

jvillian
  • 19,953
  • 5
  • 31
  • 44
1

You need to create unique index in your database table. There might be 2 processes getting to create condition together. The only way to stop these duplicate records is by having uniqueness constraint at DB level.

archana
  • 1,282
  • 8
  • 11