Uniqueness activerecord validation in a list for a user

Question

I have a user model and a Request model,

class Request < ActiveRecord::Base
  belongs_to :user
  validates :user_id, presence: true
  validates :status, inclusion: { in: %w(PENDING
                                         RUNNING
                                         COMPLETED
                                         FAILED
                                         ERROR) }

  validates :status, on: :create,
                     uniqueness: { scope: :user_id,
                                   message: "There is another request for this user",
                                   conditions: -> { where(status: ['PENDING',
                                                                   'RUNNING',
                                                                   'ERROR']) }

  }

end

The idea behind the uniqueness validation is that you cannot create a Request for that user if he has another PENDING, RUNNING or ERROR request.

it 'cannot a new pending request if user allredy has another RUNNING task' do
  described_class.create!(user: user, status: 'RUNNING')

  expect { described_class.create!(user: user) }.to raise_error(ActiveRecord::RecordInvalid, 
                                                                /There is another request for this user/)
end

This test fails because It can create another Request even when another RUNNING request was created.

I guess that is because I doing something wrong on the condition.

This is the migration that creates Request model.

class CreateRequests < ActiveRecord::Migration
  def up
    create_table :requests do |t|
      t.integer :reason
      t.string  :status, default: 'PENDING', :limit => 15
      t.integer :user_id

      t.timestamps null: false
    end
  end

  def down
    drop_table :requests
  end
end

Answer 1

I would solve this by using an ActiveRecord::Enum instead. First make requests.status an integer column with an index instead of varchar.

class CreateRequests < ActiveRecord::Migration
  def change
    create_table :requests do |t|
      t.integer :reason
      t.integer :status, default: 0, index: true
      t.belongs_to :user, foreign_key: true
      t.timestamps null: false
    end
    add_index :requests, [:user_id, :status]
  end
end

You should also use t.belongs_to :user, foreign_key: true to setup a foreign key for the user association. Specifying up and down blocks is not needed - just specify the change block and ActiveRecord is smart enough to know how to reverse it on its own.

class Request < ActiveRecord::Base
  # this defaults to true in Rails 5.
  belongs_to :user, required: true
  enum status: [:pending, :running, :completed, :failed, :error]
  validate :unique_status_within_collection, on: :create

  def unique_status_within_collection
    unique_statuses = %w[ pending running error]
    if unique_statuses.includes?(self.status) && user.requests.exists?(status: unique_statuses)
      errors.add(:base, "There is another request for this user")
    end
  end
end

One huge gotcha though is that the name request already is really significant in rails and if you use the instance variable @request in the controller you're masking the incoming request object.

I would rename it something like "UserRequest" or whatever to avoid this ambiguity.

Answer 2

Your issue is that you're trying to do too much with the validates ... uniqueness: built-in method. The method assumes that the column value must be unique, not unique within a collection.

(Your attempt to define this within the conditions parameter seems reasonable, but this only actually limits the scope of records being compared against; it does not alter the definition of "unique column" to mean "unique within a list".)

I would suggest instead writing this validation as a custom method - for example, something like:

validate :unique_status_within_collection, on: :create

def unique_status_within_collection
  unique_statuses = ['PENDING', 'RUNNING', 'ERROR']
  if unique_statuses.include?(status) && Request.exists?(user: user, status: unique_statuses)
    errors.add(:status, "There is another request for this user")
  end
end