From d29d44e859c82251b77adb948a8d347599904bc8 Mon Sep 17 00:00:00 2001
From: Will Jordan <wjordan@users.noreply.github.com>
Date: Mon, 25 Sep 2017 14:53:14 -0700
Subject: [PATCH] Optimize generate_username query (#12745)

tweak darts logic for longer suffixes
---
 dashboard/test/models/user_helpers_test.rb | 31 +++++++++++-----------
 lib/cdo/user_helpers.rb                    | 28 ++++++++++---------
 2 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/dashboard/test/models/user_helpers_test.rb b/dashboard/test/models/user_helpers_test.rb
index b1f603f15cd51..09d6c2ace4acc 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, 38329, 59306].each do |suffix|
       create_user_with_username "captain_picard#{suffix}"
     end
 
     srand 0
-    assert_equal 'captain_picard905',
+    assert_equal 'captain_picard96294',
       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..57e6f347aeb84 100644
--- a/lib/cdo/user_helpers.rb
+++ b/lib/cdo/user_helpers.rb
@@ -8,7 +8,7 @@ module UserHelpers
 
   def self.generate_username(queryable, name)
     prefix = name.downcase.
-      gsub(/[^#{USERNAME_ALLOWED_CHARACTERS.source}]+/, ' ')[0..16].
+      gsub(/[^#{USERNAME_ALLOWED_CHARACTERS.source}]+/, ' ')[0..15].
       squish.
       tr(' ', '_')
 
@@ -20,27 +20,31 @@ 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|
+    (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..18]
+        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.
+    # Use a regex to filter integer suffixes from other usernames.
+    last_id = queryable.where(['username LIKE ? and username RLIKE ?', "#{prefix}%", "^#{prefix}[0-9]+$"]).
+      # Find max integer using DB functions to avoid returning all matches to the application.
+      select("MAX(CAST(SUBSTRING(`username`, #{prefix.length + 1}) as unsigned)) as `id`").first
+
+    # ActiveRecord returns a User instance, whereas Sequel returns a hash.
+    last_id = last_id.respond_to?(:id) ? last_id.id : last_id[:id]
 
     # 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
+    suffix = last_id.to_i + 1
     return "#{prefix}#{suffix}"
   end