PERF: avoid N+1 on topic view

PERF: avoid N+1 on topic view

Serializer is injecting information into cooked and reaching direct to custom fields that were not preloaded

This amends it so basic post serializer can use the proper interface

That said we should probably follow this up so we don’t reach for this info on every post.

diff --git a/app/serializers/basic_post_serializer.rb b/app/serializers/basic_post_serializer.rb
index 9d57a1c..32c14b5 100644
--- a/app/serializers/basic_post_serializer.rb
+++ b/app/serializers/basic_post_serializer.rb
@@ -10,6 +10,8 @@ class BasicPostSerializer < ApplicationSerializer
              :cooked,
              :cooked_hidden
 
+  attr_accessor :topic_view
+
   def name
     object.user && object.user.name
   end
@@ -41,20 +43,27 @@ class BasicPostSerializer < ApplicationSerializer
       cooked = object.filter_quotes(@parent_post)
 
       if scope&.user
-        group = Group
-          .joins('JOIN group_users ON groups.id = group_users.group_id')
-          .find_by(
-            id: object.custom_fields['requested_group_id'].to_i,
-            group_users: { user_id: scope.user.id, owner: true }
-          )
-
-        if group
-          cooked << <<~EOF
-            <hr />
-            <a href="#{Discourse.base_uri}/g/#{group.name}/requests">
-              #{I18n.t('groups.request_membership_pm.handle')}
-            </a>
-          EOF
+
+        # PERF: this should not run on every post, only specific ones
+        # also, why is this in basic post serializer?
+        requested_group_id = post_custom_fields['requested_group_id'].to_i
+
+        if requested_group_id > 0
+          group = Group
+            .joins('JOIN group_users ON groups.id = group_users.group_id')
+            .find_by(
+              id: object.custom_fields['requested_group_id'].to_i,
+              group_users: { user_id: scope.user.id, owner: true }
+            )
+
+          if group
+            cooked << <<~EOF
+              <hr />
+              <a href="#{Discourse.base_uri}/g/#{group.name}/requests">
+                #{I18n.t('groups.request_membership_pm.handle')}
+              </a>
+            EOF
+          end
         end
       end
 
@@ -66,4 +75,12 @@ class BasicPostSerializer < ApplicationSerializer
     SiteSetting.enable_names?
   end
 
+  def post_custom_fields
+    @post_custom_fields ||= if @topic_view
+      (@topic_view.post_custom_fields || {})[object.id] || {}
+    else
+      object.custom_fields
+    end
+  end
+
 end
diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb
index 2221359..7ab24ca 100644
--- a/app/serializers/post_serializer.rb
+++ b/app/serializers/post_serializer.rb
@@ -4,7 +4,6 @@ class PostSerializer < BasicPostSerializer
 
   # To pass in additional information we might need
   INSTANCE_VARS ||= [
-    :topic_view,
     :parent_post,
     :add_raw,
     :add_title,
@@ -490,12 +489,4 @@ private
     @post_actions ||= (@topic_view&.all_post_actions || {})[object.id]
   end
 
-  def post_custom_fields
-    @post_custom_fields ||= if @topic_view
-      (@topic_view.post_custom_fields || {})[object.id] || {}
-    else
-      object.custom_fields
-    end
-  end
-
 end

GitHub sha: accbbded

2 Likes

@nbianca I am very unclear about this feature, why are we injecting this in the serializer, is it not simply some conditional logic on private messages that is bound to topic? Why is this bound at the post level?

Should it not be?

  • IF is topic
  • AND is PM
  • reach into topic custom fields and inform client it is an invite for “group X”

Then the client can render whatever.

It feels like a pretty heavy approach here, plus why is it bound to posts?

2 Likes

This commit has been mentioned on Discourse Meta. There might be relevant details there:

That is how I first implemented this, but it added some additional fields in the current user payload (a list of owned groups) and @tgxworld suggested to show the membership request link in the TopicViewSerializer. I liked this suggestion because it simplified a lot of things.

I did not realise that it would introduce a N+1, under certain conditions, especially since I have added it to the list of preloaded custom fields.

1 Like

Hmm but why does this tie to a post and to markdown? Is it not something that belongs on the topic? I don’t understand why it is a post custom field and not a topic custom field?

2 Likes

@SamSaffron or @davidtaylorhq, can you please approve this? :blush: It was followed up in:

1 Like