PERF: Move mention lookups out of the V8 context. (#6640)

We were looking up each mention one by one without any form of caching and that results
in a problem somewhat similar to an N+1. When we have to do alot of DB
lookups, it also increased the time spent in the V8 context which may
eventually lead to a timeout. The change here makes it such that mention lookups only does a single
DB query per post that happens outside of the V8 context.

app/assets/javascripts/pretty-text/engines/discourse-markdown/mentions.js.es6

@@ -1,32 +1,12 @@
 function addMention(buffer, matches, state) {
   let username = matches[1] || matches[2];
-  let { getURL, mentionLookup, formatUsername } = state.md.options.discourse;
-
-  let type = mentionLookup && mentionLookup(username);
-
-  let tag = "a";
+  let tag = "span";
   let className = "mention";
-  let href = null;
-
-  if (type === "user") {
-    href = getURL("/u/") + username.toLowerCase();
-  } else if (type === "group") {
-    href = getURL("/groups/") + username;
-    className = "mention-group";
-  } else {
-    tag = "span";
-  }
 
   let token = new state.Token("mention_open", tag, 1);
   token.attrs = [["class", className]];
-  if (href) {
-    token.attrs.push(["href", href]);
-  }
 
   buffer.push(token);
-  if (formatUsername) {
-    username = formatUsername(username);
-  }
 
   token = new state.Token("text", "", 0);
   token.content = "@" + username;

app/assets/javascripts/pretty-text/pretty-text.js.es6

@@ -31,7 +31,6 @@ export function buildOptions(state) {
     previewing,
     linkify,
     censoredWords,
-    mentionLookup,
     invalidateOneboxes
   } = state;
 
@@ -67,7 +66,6 @@ export function buildOptions(state) {
     lookupAvatarByPostNumber,
     lookupPrimaryUserGroupByPostNumber,
     formatUsername,
-    mentionLookup,
     emojiUnicodeReplacer,
     lookupInlineOnebox,
     lookupImageUrls,

lib/pretty_text.rb

@@ -156,7 +156,6 @@ module PrettyText
         __optInput.formatUsername = __formatUsername;
         __optInput.getTopicInfo = __getTopicInfo;
         __optInput.categoryHashtagLookup = __categoryLookup;
-        __optInput.mentionLookup = __mentionLookup;
         __optInput.customEmoji = #{custom_emoji.to_json};
         __optInput.emojiUnicodeReplacer = __emojiUnicodeReplacer;
         __optInput.lookupInlineOnebox = __lookupInlineOnebox;
@@ -265,6 +264,8 @@ module PrettyText
       add_s3_cdn(doc)
     end
 
+    add_mentions(doc) if SiteSetting.enable_mentions
+
     doc.to_html
   end
 
@@ -419,4 +420,67 @@ module PrettyText
     end
   end
 
+  private
+
+  USER_TYPE ||= 'user'
+  GROUP_TYPE ||= 'group'
+
+  def self.add_mentions(doc)
+    elements = doc.css("span.mention")
+    names = elements.map { |element| element.text[1..-1] }
+
+    mentions = lookup_mentions(names)
+
+    doc.css("span.mention").each do |element|
+      name = element.text[1..-1]
+      name.downcase!
+
+      if type = mentions[name]
+        element.name = 'a'
+
+        element.children = PrettyText::Helpers.format_username(
+          element.children.text
+        )
+
+        case type
+        when USER_TYPE
+          element['href'] = "/u/#{name}"
+        when GROUP_TYPE
+          element['class'] = 'mention-group'
+          element['href'] = "/groups/#{name}"
+        end
+      end
+    end
+  end
+
+  def self.lookup_mentions(names)
+    sql = <<~SQL
+    (
+      SELECT
+        :user_type AS type,
+        username_lower AS name
+      FROM users
+      WHERE username_lower IN (:names)
+    )
+    UNION
+    (
+      SELECT
+        :group_type AS type,
+        name
+      FROM groups
+      WHERE name IN (:names)
+    )
+    SQL
+
+    results = DB.query(sql,
+      names: names,
+      user_type: USER_TYPE,
+      group_type: GROUP_TYPE
+    )
+
+    mentions = {}
+    results.each { |result| mentions[result.name] = result.type }
+    mentions
+  end
+
 end

lib/pretty_text/helpers.rb

@@ -39,12 +39,6 @@ module PrettyText
       username
     end
 
-    def mention_lookup(name)
-      return false   if name.blank?
-      return "user"  if User.exists?(username_lower: name.downcase)
-      return "group" if Group.exists?(name: name)
-    end
-
     def category_hashtag_lookup(category_slug)
       if category = Category.query_from_hashtag_slug(category_slug)
         [category.url_with_id, category_slug]

lib/pretty_text/shims.js

@@ -1,20 +1,26 @@
-__PrettyText = require('pretty-text/pretty-text').default;
-__buildOptions = require('pretty-text/pretty-text').buildOptions;
-__performEmojiUnescape = require('pretty-text/emoji').performEmojiUnescape;
+__PrettyText = require("pretty-text/pretty-text").default;
+__buildOptions = require("pretty-text/pretty-text").buildOptions;
+__performEmojiUnescape = require("pretty-text/emoji").performEmojiUnescape;
 
-__utils = require('discourse/lib/utilities');
+__utils = require("discourse/lib/utilities");
 
 __emojiUnicodeReplacer = null;
 
 __setUnicode = function(replacements) {
-  let unicodeRegexp = new RegExp(Object.keys(replacements).sort().reverse().join("|"), "g");
+  let unicodeRegexp = new RegExp(
+    Object.keys(replacements)
+      .sort()
+      .reverse()
+      .join("|"),
+    "g"
+  );
 
   __emojiUnicodeReplacer = function(text) {
     unicodeRegexp.lastIndex = 0;
     let m;
     while ((m = unicodeRegexp.exec(text)) !== null) {
       let replacement = ":" + replacements[m[0]] + ":";
-      const before = text.charAt(m.index-1);
+      const before = text.charAt(m.index - 1);
       if (!/\B/.test(before)) {
         replacement = "\u200b" + replacement;
       }
@@ -23,7 +29,7 @@ __setUnicode = function(replacements) {
 
     // fixes Safari VARIATION SELECTOR-16 issue with some emojis
     // https://meta.discourse.org/t/emojis-selected-on-ios-displaying-additional-rectangles/86132
-    text = text.replace(/\ufe0f/g, '');
+    text = text.replace(/\ufe0f/g, "");
 
     return text;
   };
@@ -35,9 +41,13 @@ function __getURLNoCDN(url) {
   if (!url) return url;
 
   // if it's a non relative URL, return it.
-  if (url !== '/' && !/^\/[^\/]/.test(url)) { return url; }
+  if (url !== "/" && !/^\/[^\/]/.test(url)) {
+    return url;
+  }
 
-  if (url.indexOf(__paths.baseUri) !== -1) { return url; }
+  if (url.indexOf(__paths.baseUri) !== -1) {
+    return url;
+  }
   if (url[0] !== "/") url = "/" + url;
 
   return __paths.baseUri + url;
@@ -76,12 +86,11 @@ function __categoryLookup(c) {
   return __helpers.category_tag_hashtag_lookup(c);
 }
 
-function __mentionLookup(u) {
-  return __helpers.mention_lookup(u);
-}
-
 function __lookupAvatar(p) {
-  return __utils.avatarImg({size: "tiny", avatarTemplate: __helpers.avatar_template(p) }, __getURL);
+  return __utils.avatarImg(
+    { size: "tiny", avatarTemplate: __helpers.avatar_template(p) },
+    __getURL
+  );
 }
 
 function __formatUsername(username) {
@@ -97,5 +106,7 @@ function __getCurrentUser(userId) {
 }
 
 I18n = {
-  t: function(a,b) { return __helpers.t(a,b); }
+  t: function(a, b) {
+    return __helpers.t(a, b);
+  }
 };

spec/components/pretty_text_spec.rb

@@ -220,18 +220,37 @@ describe PrettyText do
       expect(PrettyText.cook("hi\n@.s.s")).to eq("<p>hi<br>\n@.s.s</p>")
     end
 
-    it "can handle mention with hyperlinks" do
-      Fabricate(:user, username: "sam")
-      expect(PrettyText.cook("hi @sam! hi")).to match_html '<p>hi <a class="mention" href="/u/sam">@sam</a>! hi</p>'
-      expect(PrettyText.cook("hi\n@sam.")).to eq("<p>hi<br>\n<a class=\"mention\" href=\"/u/sam\">@sam</a>.</p>")
+    it "handles user and group mentions correctly" do
+      ['user', 'user2'].each do |username |
+        Fabricate(:user, username: username)
       end
 
-    it "can handle group mention" do
       group = Fabricate(:group)
 
-      expect(PrettyText.cook("hi @#{group.name}! hi")).to match_html(
-        %Q{<p>hi <a class="mention-group" href="/groups/#{group.name}">@#{group.name}</a>! hi</p>}
-      )
+      [
+        [
+          'hi @user! @user2 hi',
+          '<p>hi <a class="mention" href="/u/user">@user</a>! <a class="mention" href="/u/user2">@user2</a> hi</p>'
+        ],
+        [
+          "hi\n@user. @#{group.name} @somemention",
+          %Q|<p>hi<br>\n<a class="mention" href="/u/user">@user</a>. <a class="mention-group" href="/groups/#{group.name}">@#{group.name}</a> <span class="mention">@somemention</span></p>|
+        ]
+      ].each do |input, expected|
+        expect(PrettyText.cook(input)).to eq(expected)
+      end
+    end
+
+    describe 'when mentions are disabled' do
+      before do
+        SiteSetting.enable_mentions = false
+      end
+
+      it 'should not convert mentions to links' do
+        user = Fabricate(:user)
+
+        expect(PrettyText.cook('hi @user')).to eq('<p>hi @user</p>')
+      end
     end
 
     it "can handle mentions inside a hyperlink" do

test/javascripts/lib/pretty-text-test.js.es6

@@ -435,16 +435,9 @@ QUnit.test("Quotes", assert => {
 });
 
 QUnit.test("Mentions", assert => {
-  const alwaysTrue = {
-    mentionLookup: function() {
-      return "user";
-    }
-  };
-
-  assert.cookedOptions(
+  assert.cooked(
     "Hello @sam",
-    alwaysTrue,
-    '<p>Hello <a class="mention" href="/u/sam">@sam</a></p>',
+    '<p>Hello <span class="mention">@sam</span></p>',
     "translates mentions to links"
   );
 
@@ -454,9 +447,8 @@ QUnit.test("Mentions", assert => {
     "it doesn't do mentions within links"
   );
 
-  assert.cookedOptions(
+  assert.cooked(
     "[@codinghorror](https://twitter.com/codinghorror)",
-    alwaysTrue,
     '<p><a href="https://twitter.com/codinghorror">@codinghorror</a></p>',
     "it doesn't do link mentions within links"
   );
@@ -557,17 +549,9 @@ QUnit.test("Mentions", assert => {
     "handles mentions separated by a slash."
   );
 
-  assert.cookedOptions(
-    "@eviltrout",
-    alwaysTrue,
-    '<p><a class="mention" href="/u/eviltrout">@eviltrout</a></p>',
-    "it doesn't onebox mentions"
-  );
-
-  assert.cookedOptions(
+  assert.cooked(
     "<small>a @sam c</small>",
-    alwaysTrue,
-    '<p><small>a <a class="mention" href="/u/sam">@sam</a> c</small></p>',
+    '<p><small>a <span class="mention">@sam</span> c</small></p>',
     "it allows mentions within HTML tags"
   );
 });