Merge pull request #18061 from code-dot-org/generate_username_fix

Optimize generate_username query

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|

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 = <<SQL
+SELECT #{cast.call('username')} + 1
+  FROM users u
+  WHERE username LIKE "#{prefix}%"
+    AND username RLIKE "^#{prefix}[0-9]+$"
+    AND NOT EXISTS (
+      SELECT 1
+      FROM users u2
+      WHERE u2.username = CONCAT("#{prefix}", #{cast.call('u.username')} + 1)
+    )
+  LIMIT 1;
+SQL
+    # Execute raw query using either ActiveRecord or Sequel object.
+    next_id = queryable.respond_to?(:connection) ?
+      queryable.connection.execute(query).first.first :
+      queryable.db.fetch(query).first.values.first
+    username = "#{prefix}#{next_id}"
+    raise "generate_username overflow: #{username}" if username.length > USERNAME_MAX_LENGTH
+    username
   end
 
   def self.random_donor