Implement final review comments from @DouweM and @rymai
- Have `Uniquify` take a block instead of a Proc/function. This is more idiomatic than passing around a function in Ruby. - Block a user before moving their issues to the ghost user. This avoids a data race where an issue is created after the issues are migrated to the ghost user, and before the destroy takes place. - No need to migrate issues (to the ghost user) in a transaction, because we're using `update_all` - Other minor changes
This commit is contained in:
parent
3bd2a98f64
commit
f2ed82fa84
6 changed files with 62 additions and 61 deletions
|
@ -1,15 +1,15 @@
|
||||||
class Uniquify
|
class Uniquify
|
||||||
# Return a version of the given 'base' string that is unique
|
# Return a version of the given 'base' string that is unique
|
||||||
# by appending a counter to it. Uniqueness is determined by
|
# by appending a counter to it. Uniqueness is determined by
|
||||||
# repeated calls to `exists_fn`.
|
# repeated calls to the passed block.
|
||||||
#
|
#
|
||||||
# If `base` is a function/proc, we expect that calling it with a
|
# If `base` is a function/proc, we expect that calling it with a
|
||||||
# candidate counter returns a string to test/return.
|
# candidate counter returns a string to test/return.
|
||||||
def string(base, exists_fn)
|
def string(base)
|
||||||
@base = base
|
@base = base
|
||||||
@counter = nil
|
@counter = nil
|
||||||
|
|
||||||
increment_counter! while exists_fn[base_string]
|
increment_counter! while yield(base_string)
|
||||||
base_string
|
base_string
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -24,6 +24,7 @@ class Uniquify
|
||||||
end
|
end
|
||||||
|
|
||||||
def increment_counter!
|
def increment_counter!
|
||||||
@counter = @counter ? @counter.next : 1
|
@counter ||= 0
|
||||||
|
@counter += 1
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -99,7 +99,7 @@ class Namespace < ActiveRecord::Base
|
||||||
path = "blank" if path.blank?
|
path = "blank" if path.blank?
|
||||||
|
|
||||||
uniquify = Uniquify.new
|
uniquify = Uniquify.new
|
||||||
uniquify.string(path, -> (s) { Namespace.find_by_path_or_name(s) })
|
uniquify.string(path) { |s| Namespace.find_by_path_or_name(s) }
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -346,43 +346,7 @@ class User < ActiveRecord::Base
|
||||||
# Return (create if necessary) the ghost user. The ghost user
|
# Return (create if necessary) the ghost user. The ghost user
|
||||||
# owns records previously belonging to deleted users.
|
# owns records previously belonging to deleted users.
|
||||||
def ghost
|
def ghost
|
||||||
ghost_user = User.find_by_ghost(true)
|
User.find_by_ghost(true) || create_ghost_user
|
||||||
|
|
||||||
ghost_user ||
|
|
||||||
begin
|
|
||||||
# Since we only want a single ghost user in an instance, we use an
|
|
||||||
# exclusive lease to ensure than this block is never run concurrently.
|
|
||||||
lease_key = "ghost_user_creation"
|
|
||||||
lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
|
|
||||||
|
|
||||||
until uuid = lease.try_obtain
|
|
||||||
# Keep trying until we obtain the lease. To prevent hammering Redis too
|
|
||||||
# much we'll wait for a bit between retries.
|
|
||||||
sleep(1)
|
|
||||||
end
|
|
||||||
|
|
||||||
# Recheck if a ghost user is already present (one might have been)
|
|
||||||
# added between the time we last checked (first line of this method)
|
|
||||||
# and the time we acquired the lock.
|
|
||||||
ghost_user = User.find_by_ghost(true)
|
|
||||||
return ghost_user if ghost_user.present?
|
|
||||||
|
|
||||||
uniquify = Uniquify.new
|
|
||||||
|
|
||||||
username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
|
|
||||||
|
|
||||||
email = uniquify.string(
|
|
||||||
-> (n) { "ghost#{n}@example.com" },
|
|
||||||
-> (s) { User.find_by_email(s) }
|
|
||||||
)
|
|
||||||
|
|
||||||
User.create(
|
|
||||||
username: username, password: Devise.friendly_token,
|
|
||||||
email: email, name: "Ghost User", state: :blocked, ghost: true
|
|
||||||
)
|
|
||||||
ensure
|
|
||||||
Gitlab::ExclusiveLease.cancel(lease_key, uuid)
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -1052,4 +1016,38 @@ class User < ActiveRecord::Base
|
||||||
super
|
super
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def self.create_ghost_user
|
||||||
|
# Since we only want a single ghost user in an instance, we use an
|
||||||
|
# exclusive lease to ensure than this block is never run concurrently.
|
||||||
|
lease_key = "ghost_user_creation"
|
||||||
|
lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
|
||||||
|
|
||||||
|
until uuid = lease.try_obtain
|
||||||
|
# Keep trying until we obtain the lease. To prevent hammering Redis too
|
||||||
|
# much we'll wait for a bit between retries.
|
||||||
|
sleep(1)
|
||||||
|
end
|
||||||
|
|
||||||
|
# Recheck if a ghost user is already present. One might have been
|
||||||
|
# added between the time we last checked (first line of this method)
|
||||||
|
# and the time we acquired the lock.
|
||||||
|
ghost_user = User.find_by_ghost(true)
|
||||||
|
return ghost_user if ghost_user.present?
|
||||||
|
|
||||||
|
uniquify = Uniquify.new
|
||||||
|
|
||||||
|
username = uniquify.string("ghost") { |s| User.find_by_username(s) }
|
||||||
|
|
||||||
|
email = uniquify.string(-> (n) { "ghost#{n}@example.com" }) do |s|
|
||||||
|
User.find_by_email(s)
|
||||||
|
end
|
||||||
|
|
||||||
|
User.create(
|
||||||
|
username: username, password: Devise.friendly_token,
|
||||||
|
email: email, name: "Ghost User", state: :blocked, ghost: true
|
||||||
|
)
|
||||||
|
ensure
|
||||||
|
Gitlab::ExclusiveLease.cancel(lease_key, uuid)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -39,11 +39,16 @@ module Users
|
||||||
private
|
private
|
||||||
|
|
||||||
def move_issues_to_ghost_user(user)
|
def move_issues_to_ghost_user(user)
|
||||||
|
# Block the user before moving issues to prevent a data race.
|
||||||
|
# If the user creates an issue after `move_issues_to_ghost_user`
|
||||||
|
# runs and before the user is destroyed, the destroy will fail with
|
||||||
|
# an exception. We block the user so that issues can't be created
|
||||||
|
# after `move_issues_to_ghost_user` runs and before the destroy happens.
|
||||||
|
user.block
|
||||||
|
|
||||||
ghost_user = User.ghost
|
ghost_user = User.ghost
|
||||||
|
|
||||||
Issue.transaction do
|
|
||||||
user.issues.update_all(author_id: ghost_user.id)
|
user.issues.update_all(author_id: ghost_user.id)
|
||||||
end
|
|
||||||
|
|
||||||
user.reload
|
user.reload
|
||||||
end
|
end
|
||||||
|
|
|
@ -1,38 +1,31 @@
|
||||||
require 'spec_helper'
|
require 'spec_helper'
|
||||||
|
|
||||||
describe Uniquify, models: true do
|
describe Uniquify, models: true do
|
||||||
|
let(:uniquify) { described_class.new }
|
||||||
|
|
||||||
describe "#string" do
|
describe "#string" do
|
||||||
it 'returns the given string if it does not exist' do
|
it 'returns the given string if it does not exist' do
|
||||||
uniquify = Uniquify.new
|
result = uniquify.string('test_string') { |s| false }
|
||||||
|
|
||||||
result = uniquify.string('test_string', -> (s) { false })
|
|
||||||
|
|
||||||
expect(result).to eq('test_string')
|
expect(result).to eq('test_string')
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'returns the given string with a counter attached if the string exists' do
|
it 'returns the given string with a counter attached if the string exists' do
|
||||||
uniquify = Uniquify.new
|
result = uniquify.string('test_string') { |s| s == 'test_string' }
|
||||||
|
|
||||||
result = uniquify.string('test_string', -> (s) { true if s == 'test_string' })
|
|
||||||
|
|
||||||
expect(result).to eq('test_string1')
|
expect(result).to eq('test_string1')
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'increments the counter for each candidate string that also exists' do
|
it 'increments the counter for each candidate string that also exists' do
|
||||||
uniquify = Uniquify.new
|
result = uniquify.string('test_string') { |s| s == 'test_string' || s == 'test_string1' }
|
||||||
|
|
||||||
result = uniquify.string('test_string', -> (s) { true if s == 'test_string' || s == 'test_string1' })
|
|
||||||
|
|
||||||
expect(result).to eq('test_string2')
|
expect(result).to eq('test_string2')
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'allows passing in a base function that defines the location of the counter' do
|
it 'allows passing in a base function that defines the location of the counter' do
|
||||||
uniquify = Uniquify.new
|
result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s|
|
||||||
|
s == 'test__string'
|
||||||
result = uniquify.string(
|
end
|
||||||
-> (counter) { "test_#{counter}_string" },
|
|
||||||
-> (s) { true if s == 'test__string' }
|
|
||||||
)
|
|
||||||
|
|
||||||
expect(result).to eq('test_1_string')
|
expect(result).to eq('test_1_string')
|
||||||
end
|
end
|
||||||
|
|
|
@ -47,6 +47,10 @@ describe Users::DestroyService, services: true do
|
||||||
|
|
||||||
expect(migrated_issue.author).to eq(User.ghost)
|
expect(migrated_issue.author).to eq(User.ghost)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'blocks the user before migrating issues to the "Ghost User' do
|
||||||
|
expect(user).to be_blocked
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context "for an issue the user was assigned to" do
|
context "for an issue the user was assigned to" do
|
||||||
|
|
Loading…
Reference in a new issue