Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix races that lead to duplicate votes creation #211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Change log

## master (unreleased)

### Bug Fixes

* [#127](https://github.com/ryanto/acts_as_votable/issues/127): Fix races that lead to duplicate votes creation. ([@fatkodima][])

For this to work, you need to update your database schema and existing data.
```ruby
# 1. Add a new column to the `votes` table.
add_column :votes, :uniqueness_token, :string

# 2. Manually remove erroneously created duplicate votes.

# 3. If you are using `duplicate: true` while voting in your codebase,
# update all those duplicated votes by setting a unique `uniqueness_token` for each of them,
# something like a unique hex value would suffice.

# 4. For other non duplicated votes update all of them setting the
# `uniqueness_token` field to "unique vote" string value.

# 5. Update `uniqueness_token` column to disallow NULL values.
change_column_null :votes, :uniqueness_token, false

# 6. Add a new unique index
add_index :votes, [:voter_id, :voter_type, :vote_scope, :votable_id, :votable_type, :uniqueness_token],
name: "index_votes_uniqueness", unique: true

# 7. Now you can delete the old index
remove_index :votes, [:voter_id, :voter_type, :vote_scope]
```

NOTE: Make sure you adapted that steps for your workload and run that commands safely
(adding/removing indexes concurrently, updating columns in batches, etc).

[@fatkodima]: https://github.com/fatkodima
10 changes: 10 additions & 0 deletions lib/acts_as_votable/votable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ def default_conditions
}
end

UNIQUE_VOTE_TOKEN = "unique vote"
private_constant :UNIQUE_VOTE_TOKEN

# voting
def vote_by(args = {})
return false if args[:voter].nil?
Expand All @@ -81,6 +84,13 @@ def vote_by(args = {})
voter: options[:voter],
vote_scope: options[:vote_scope]
)

if options[:duplicate]
require "securerandom"
vote.uniqueness_token = SecureRandom.hex
else
vote.uniqueness_token = UNIQUE_VOTE_TOKEN
end
else
# this voter is potentially changing his vote
vote = votes.last
Expand Down
5 changes: 3 additions & 2 deletions lib/acts_as_votable/vote.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ class Vote < ::ActiveRecord::Base
scope :for_type, ->(klass) { where(votable_type: klass.to_s) }
scope :by_type, ->(klass) { where(voter_type: klass.to_s) }

validates_presence_of :votable_id
validates_presence_of :voter_id
validates :votable_id, presence: true
validates :voter_id, presence: true,
uniqueness: { scope: [:voter_type, :vote_scope, :votable, :uniqueness_token] }
end
end
Original file line number Diff line number Diff line change
@@ -1,22 +1,20 @@
class ActsAsVotableMigration < ActiveRecord::Migration<%= migration_version %>
def self.up
def change
create_table :votes do |t|
t.references :votable, null: false, polymorphic: true, index: false
t.references :voter, null: false, polymorphic: true, index: false

t.references :votable, :polymorphic => true
t.references :voter, :polymorphic => true

t.boolean :vote_flag
t.boolean :vote_flag, null: false, default: true
t.string :vote_scope
t.integer :vote_weight
t.integer :vote_weight, null: false, default: 1
t.string :uniqueness_token, null: false # used for 'duplicate' feature

t.timestamps
end

add_index :votes, [:voter_id, :voter_type, :vote_scope]
add_index :votes, [:votable_id, :votable_type, :vote_scope]
end
t.index [:voter_id, :voter_type, :vote_scope, :votable_id, :votable_type, :uniqueness_token],
name: "index_votes_uniqueness", unique: true

def self.down
drop_table :votes
t.index [:votable_id, :votable_type, :vote_scope]
end
end
end
15 changes: 8 additions & 7 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,18 @@
t.references :votable, polymorphic: true
t.references :voter, polymorphic: true

t.boolean :vote_flag
t.boolean :vote_flag, null: false, default: true
t.string :vote_scope
t.integer :vote_weight
t.integer :vote_weight, null: false, default: 1
t.string :uniqueness_token, null: false

t.timestamps
end

add_index :votes, [:votable_id, :votable_type]
add_index :votes, [:voter_id, :voter_type]
add_index :votes, [:voter_id, :voter_type, :vote_scope]
add_index :votes, [:votable_id, :votable_type, :vote_scope]
t.index [:voter_id, :voter_type, :vote_scope, :votable_id, :votable_type, :uniqueness_token],
name: "index_votes_uniqueness", unique: true

t.index [:votable_id, :votable_type, :vote_scope]
end

create_table :voters do |t|
t.string :name
Expand Down