From e44b21ad2e90d92d39279327b6d5194c5d2da1bf Mon Sep 17 00:00:00 2001 From: Will Jordan Date: Mon, 25 Sep 2017 14:53:14 -0700 Subject: [PATCH] Optimize generate_username query Use more accurate MySQL query for finding gaps in integer sequence. --- dashboard/test/models/user_helpers_test.rb | 31 +++++++------- lib/cdo/user_helpers.rb | 48 ++++++++++++++-------- 2 files changed, 47 insertions(+), 32 deletions(-) diff --git a/dashboard/test/models/user_helpers_test.rb b/dashboard/test/models/user_helpers_test.rb index b1f603f15cd51..78dc562a0da19 100644 --- a/dashboard/test/models/user_helpers_test.rb +++ b/dashboard/test/models/user_helpers_test.rb @@ -36,39 +36,38 @@ def create_user_with_username(username) test 'generate_username for existing username via dart throwing' do create_user_with_username 'captain_picard' - # An existing username attempts the username, fails, and receives '6'. + # An existing username attempts the username, fails, and receives '784'. srand 0 - assert_equal 'captain_picard6', + assert_equal 'captain_picard784', UserHelpers.generate_username(User, 'Captain Picard') - create_user_with_username 'captain_picard6' + create_user_with_username 'captain_picard784' - # The next Captain Picard attempts '6', fails, and receives '1' + # The next Captain Picard attempts '784', fails, and receives '659' srand 0 - assert_equal 'captain_picard1', + assert_equal 'captain_picard659', UserHelpers.generate_username(User, 'Captain Picard') - create_user_with_username 'captain_picard1' + create_user_with_username 'captain_picard659' - # The next Captain Picard attempts '6', fails, attempts '1', fails, and - # receives '4'. + # The next Captain Picard attempts '784' and '659', fails, and + # receives '4264'. srand 0 - assert_equal 'captain_picard4', + assert_equal 'captain_picard4264', UserHelpers.generate_username(User, 'Captain Picard') - create_user_with_username 'captain_picard4' + create_user_with_username 'captain_picard4264' - # The next Captain Picard attempts '6', fails, attempts '1', fails, - # attempts '4', fails, and receives '77'. + # The next Captain Picard attempts the above, fails and receives '5859'. srand 0 - assert_equal 'captain_picard77', + assert_equal 'captain_picard5859', UserHelpers.generate_username(User, 'Captain Picard') end test 'generate_username for existing username via fallback' do - ['', '6', '1', '4', '77', '19', '93', '377', '854', '904'].each do |suffix| + ['', 784, 659, 4264, 5859, 51993, 96293, 54824, 47456, 383298, 593063, 548242, 474564].each do |suffix| create_user_with_username "captain_picard#{suffix}" end srand 0 - assert_equal 'captain_picard905', + assert_equal 'captain_picard383299', UserHelpers.generate_username(User, 'Captain Picard') end @@ -98,7 +97,7 @@ def create_user_with_username(username) test 'generate_username' do default_params = {email: 'foo@bar.com', password: 'foosbars', name: 'tester', user_type: User::TYPE_STUDENT, age: 28} names = ['a', 'b', 'Captain Picard', 'Captain Picard', 'Captain Picard', 'this is a really long name blah blah blah blah blah blah'] - expected_usernames = %w(coder_a coder_b captain_picard captain_picard6 captain_picard1 this_is_a_really) + expected_usernames = %w(coder_a coder_b captain_picard captain_picard784 captain_picard659 this_is_a_really) i = 0 users = names.map do |name| diff --git a/lib/cdo/user_helpers.rb b/lib/cdo/user_helpers.rb index 54061fbbe2f0b..ef7d5369638e7 100644 --- a/lib/cdo/user_helpers.rb +++ b/lib/cdo/user_helpers.rb @@ -5,10 +5,11 @@ module UserHelpers # * uniqueness subject to race conditions. There's a unique # constraint in the db -- callers should retry USERNAME_ALLOWED_CHARACTERS = /[a-z0-9\-\_\.]/ + USERNAME_MAX_LENGTH = 20 def self.generate_username(queryable, name) prefix = name.downcase. - gsub(/[^#{USERNAME_ALLOWED_CHARACTERS.source}]+/, ' ')[0..16]. + gsub(/[^#{USERNAME_ALLOWED_CHARACTERS.source}]+/, ' ')[0..USERNAME_MAX_LENGTH - 5]. squish. tr(' ', '_') @@ -19,29 +20,44 @@ def self.generate_username(queryable, name) return prefix if queryable.where(username: prefix).limit(1).empty? - # Throw darts to find an appropriate suffix, using it if we hit bullseye. - (0..2).each do |exponent| + # Throw random darts of increasing length (3 to 7 digits) to find an unused suffix. + (2..6).each do |exponent| min_index = 10**exponent max_index = 10**(exponent + 1) - 1 - 3.times do |_i| + 2.times do |_i| suffix = Random.rand(min_index..max_index) - if queryable.where(username: "#{prefix}#{suffix}").limit(1).empty? - return "#{prefix}#{suffix}" + # Truncate generated username to max allowed length. + username = "#{prefix}#{suffix}"[0..USERNAME_MAX_LENGTH - 1] + if queryable.where(username: username).limit(1).empty? + return username end end end - # The darts missed, so revert to using a slow query. - similar_users = queryable.where(["username like ?", prefix + '%']).select(:username).to_a - similar_usernames = similar_users.map do |user| - # ActiveRecord returns a User instance, whereas Sequel returns a hash. - user.respond_to?(:username) ? user.username : user[:username] - end + # Fallback to a range-scan query to find an available gap in the integer sequence. + + # Use CAST() and SUBSTRING() to parse the suffix as an integer. + cast = lambda {|t| "CAST(SUBSTRING(#{t}, #{prefix.length + 1}) as unsigned)"} - # Increment the current maximum integer suffix. Though it may leave holes, - # it is guaranteed to be (currently) unique. - suffix = similar_usernames.map {|n| n[prefix.length..-1]}.map(&:to_i).max + 1 - return "#{prefix}#{suffix}" + query = < USERNAME_MAX_LENGTH + username end def self.random_donor