consul/grecia
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Add unique index to poll voters table

Browse Source 
Note that Rails 7.1 changes `find_or_create_by!` so it calls
`create_or_find_by!` when no record is found, meaning we'll rarely get
`RecordNotUnique` exceptions when using this method during a race
condition.

Adding this index means we need to remove the uniqueness validation.
According to the `create_or_find_by` documentation [1]:

> Columns with unique database constraints should not have uniqueness
> validations defined, otherwise create will fail due to validation
> errors and find_by will never be called.

We're adding a test that checks what happens when using
`create_or_find_by!`.

Note that we're creating voters combining `create_with` with
`find_or_create_by!`. Using `find_or_create_by!(...)` with all
attributes (including non-key ones like `origin`) fails when a voter
already exists with different values, e.g. an existing `origin: "web"`
and an incoming `"booth"`. In this situation the existing record is not
matched and the unique index raises an exception.

`create_with(...).find_or_create_by!(user: ..., poll: ...)` searches by
the unique key only and applies the extra attributes only on creation.
Existing voters are returned unchanged, which is the intended behavior.

For the `take_votes_from` method, we're handling a (highly unlikely, but
theoretically possible) scenario where a user votes at the same time as
taking voters from another user. For that, we're doing something similar
to what `create_or_find_by!` does: we're updating the `user_id` column
inside a new transaction (using a new transactions avoids a
`PG::InFailedSqlTransaction` exception when there are duplicate
records), and deleting the existing voter when we get a
`RecordNotUnique` exception.

On `Poll::WebVote` we're simply raising an exception when there's
already a user who's voted via booth, because the `Poll::WebVote#update`
method should never be called in this case.

We still need to use `with_lock` in `Poll::WebVote`, but not due to
duplicate voters (`find_or_create_by!` method will now handle the unique
record scenario, even in the case of simultaneous transactions), but
because we use a uniqueness validation in `Poll::Answer`; this
validation would cause an error in simultaneous transactions.

[1] https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Relation.html#method-i-create_or_find_by
This commit is contained in:
Javi Martín
2024-05-10 22:55:56 +02:00
parent 03c5533cf0
commit 6da53b5716
11 changed files with 64 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
app/controllers/officing/voters_controller.rb
View File

@@ -14,18 +14,14 @@ class Officing::VotersController < Officing::BaseController
@poll = Poll.find(voter_params[:poll_id])
@user = User.find(voter_params[:user_id])
@user.with_lock do
@voter = Poll::Voter.new(document_type: @user.document_type,
document_number: @user.document_number,
user: @user,
poll: @poll,
origin: "booth",
officer: current_user.poll_officer,
booth_assignment: current_booth.booth_assignments.find_by(poll: @poll),
officer_assignment: officer_assignment(@poll))
@voter.save!
end
@voter = Poll::Voter.create_with(
document_type: @user.document_type,
document_number: @user.document_number,
origin: "booth",
officer: current_user.poll_officer,
booth_assignment: current_booth.booth_assignments.find_by(poll: @poll),
officer_assignment: officer_assignment(@poll)
).find_or_create_by!(user: @user, poll: @poll)
end
private

1
app/models/poll/voter.rb
View File

@@ -15,7 +15,6 @@ class Poll
validates :officer_assignment_id, presence: true, if: ->(voter) { voter.origin == "booth" }
validates :document_number, presence: true, unless: :skip_user_verification?
validates :user_id, uniqueness: { scope: [:poll_id], message: :has_voted }
validates :origin, inclusion: { in: ->(*) { VALID_ORIGINS }}
before_validation :set_demographic_info, :set_document_info, :set_denormalized_booth_assignment_id

12
app/models/user.rb
View File

@@ -289,14 +289,10 @@ class User < ApplicationRecord
def take_votes_from(other_user)
return if other_user.blank?
with_lock do
Poll::Voter.where(user_id: other_user.id).find_each do |poll_voter|
if Poll::Voter.where(poll: poll_voter.poll, user_id: id).any?
poll_voter.delete
else
poll_voter.update_column(:user_id, id)
end
end
Poll::Voter.where(user_id: other_user.id).find_each do |poll_voter|
transaction(requires_new: true) { poll_voter.update_column(:user_id, id) }
rescue ActiveRecord::RecordNotUnique
poll_voter.delete
end
Budget::Ballot.where(user_id: other_user.id).update_all(user_id: id)

2
config/locales/en/activerecord.yml
View File

@@ -576,8 +576,6 @@ en:
attributes:
document_number:
not_in_census: "Document not in census"
user_id:
has_voted: "User has already voted"
legislation/process:
attributes:
end_date:

2
config/locales/es/activerecord.yml
View File

@@ -576,8 +576,6 @@ es:
attributes:
document_number:
not_in_census: "Este documento no aparece en el censo"
user_id:
has_voted: "Este usuario ya ha votado"
legislation/process:
attributes:
end_date:

5
db/migrate/20250814122241_add_unique_index_to_poll_voters.rb Normal file
View File

@@ -0,0 +1,5 @@
class AddUniqueIndexToPollVoters < ActiveRecord::Migration[7.1]
def change
add_index :poll_voters, [:user_id, :poll_id], unique: true
end
end

3
db/schema.rb
View File

@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema[7.1].define(version: 2025_03_13_014205) do
ActiveRecord::Schema[7.1].define(version: 2025_08_14_122241) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_trgm"
enable_extension "plpgsql"
@@ -1209,6 +1209,7 @@ ActiveRecord::Schema[7.1].define(version: 2025_03_13_014205) do
t.index ["officer_assignment_id"], name: "index_poll_voters_on_officer_assignment_id"
t.index ["poll_id", "document_number", "document_type"], name: "doc_by_poll"
t.index ["poll_id"], name: "index_poll_voters_on_poll_id"
t.index ["user_id", "poll_id"], name: "index_poll_voters_on_user_id_and_poll_id", unique: true
t.index ["user_id"], name: "index_poll_voters_on_user_id"
end

24
spec/controllers/officing/voters_controller_spec.rb
View File

@@ -2,20 +2,19 @@ require "rails_helper"
describe Officing::VotersController do
describe "POST create" do
let(:officer) { create(:poll_officer) }
before { sign_in(officer.user) }
it "does not create two records with two simultaneous requests", :race_condition do
officer = create(:poll_officer)
poll = create(:poll, officers: [officer])
user = create(:user, :level_two)
sign_in(officer.user)
2.times.map do
Thread.new do
post :create, params: {
voter: { poll_id: poll.id, user_id: user.id },
format: :js
}
rescue ActiveRecord::RecordInvalid
end
end.each(&:join)
@@ -24,7 +23,6 @@ describe Officing::VotersController do
end
it "stores officer and booth information" do
officer = create(:poll_officer)
user = create(:user, :in_census)
poll1 = create(:poll, name: "Would you be interested in XYZ?")
poll2 = create(:poll, name: "Testing polls")
@@ -37,7 +35,6 @@ describe Officing::VotersController do
validate_officer
set_officing_booth(booth)
sign_in(officer.user)
post :create, params: {
voter: { poll_id: poll1.id, user_id: user.id },
@@ -61,5 +58,20 @@ describe Officing::VotersController do
expect(voter2.booth_assignment).to eq(assignment2)
expect(voter2.officer_assignment).to eq(assignment2.officer_assignments.first)
end
it "does not overwrite non key attributes when a web voter already exists" do
user = create(:user, :level_two, document_number: "11223344Z")
poll = create(:poll, officers: [officer])
existing = create(:poll_voter, poll: poll, user: user, origin: "web")
expect do
post :create, params: { voter: { poll_id: poll.id, user_id: user.id }, format: :js }
expect(response).to be_successful
end.not_to change { Poll::Voter.count }
existing.reload
expect(existing.origin).to eq "web"
expect(existing.document_number).to eq "11223344Z"
end
end
end

43
spec/lib/tasks/polls_spec.rb
View File

@@ -4,49 +4,6 @@ describe "polls tasks" do
let(:poll) { create(:poll) }
let(:user) { create(:user, :level_two) }
describe "polls:remove_duplicate_voters" do
before { Rake::Task["polls:remove_duplicate_voters"].reenable }
it "removes duplicate voters" do
second_user = create(:user, :level_two)
voter = create(:poll_voter, poll: poll, user: user)
second_voter = create(:poll_voter, poll: poll, user: second_user)
other_user_voter = create(:poll_voter, poll: poll, user: create(:user, :level_two))
other_poll_voter = create(:poll_voter, poll: create(:poll), user: user)
2.times { insert(:poll_voter, poll_id: poll.id, user_id: user.id) }
insert(:poll_voter, poll_id: poll.id, user_id: second_user.id)
expect(Poll::Voter.count).to eq 7
Rake.application.invoke_task("polls:remove_duplicate_voters")
expect(Poll::Voter.count).to eq 4
expect(Poll::Voter.all).to match_array [voter, second_voter, other_user_voter, other_poll_voter]
end
it "removes duplicate voters on tenants" do
create(:tenant, schema: "voters")
Tenant.switch("voters") do
poll = create(:poll)
user = create(:user, :level_two)
create(:poll_voter, poll: poll, user: user)
insert(:poll_voter, poll_id: poll.id, user_id: user.id)
expect(Poll::Voter.count).to eq 2
end
Rake.application.invoke_task("polls:remove_duplicate_voters")
Tenant.switch("voters") do
expect(Poll::Voter.count).to eq 1
end
end
end
describe "polls:remove_duplicate_answers" do
before { Rake::Task["polls:remove_duplicate_answers"].reenable }

24
spec/models/poll/voter_spec.rb
View File

@@ -33,8 +33,7 @@ describe Poll::Voter do
voter = build(:poll_voter, user: user, poll: poll)
expect(voter).not_to be_valid
expect(voter.errors.messages[:user_id]).to eq(["User has already voted"])
expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique
end
it "is not valid if the user has already voted in the same poll/booth" do
@@ -42,8 +41,7 @@ describe Poll::Voter do
voter = build(:poll_voter, user: user, poll: poll, booth_assignment: booth_assignment)
expect(voter).not_to be_valid
expect(voter.errors.messages[:user_id]).to eq(["User has already voted"])
expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique
end
it "is not valid if the user has already voted in different booth in the same poll" do
@@ -51,8 +49,7 @@ describe Poll::Voter do
voter = build(:poll_voter, :from_booth, user: user, poll: poll, booth: booth)
expect(voter).not_to be_valid
expect(voter.errors.messages[:user_id]).to eq(["User has already voted"])
expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique
end
it "is valid if the user has already voted in the same booth in different poll" do
@@ -68,8 +65,8 @@ describe Poll::Voter do
create(:poll_voter, :from_web, user: answer.author, poll: answer.poll)
voter = build(:poll_voter, poll: answer.question.poll, user: answer.author)
expect(voter).not_to be_valid
expect(voter.errors.messages[:user_id]).to eq(["User has already voted"])
expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique
end
context "Skip verification is enabled" do
@@ -83,7 +80,7 @@ describe Poll::Voter do
voter = build(:poll_voter, user: user, poll: poll)
expect(voter).not_to be_valid
expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique
end
it "is valid if other users have voted in the same poll" do
@@ -146,6 +143,15 @@ describe Poll::Voter do
expect(voter).to be_valid
end
end
describe ".create_or_find_by!" do
it "finds the voter when it already exists instead of failing the validation" do
existing_voter = Poll::Voter.create_with(origin: "web")
.create_or_find_by!(user: voter.user, poll: voter.poll)
expect(existing_voter).to eq voter
end
end
end
describe "scopes" do

12
spec/models/poll/web_vote_spec.rb
View File

@@ -73,6 +73,18 @@ describe Poll::WebVote do
expect(question.reload.answers.size).to eq 0
end
it "raises an exception when the user has already voted in a booth" do
create(:poll_voter, :from_booth, poll: poll, user: user)
expect do
web_vote.update(question.id.to_s => { option_id: option_yes.id.to_s })
end.to raise_error(ActiveRecord::RecordNotFound)
expect(poll.reload.voters.size).to eq 1
expect(poll.voters.first.user).to eq user
expect(question.reload.answers.size).to eq 0
end
context "creating answers at the same time", :race_condition do
it "does not create two voters or two answers for two different answers" do
[option_yes, option_no].map do |option|