From 50a95ecfa24ddea79b8ce01cb2ea4c6a6b555dca Mon Sep 17 00:00:00 2001 From: Guilherme Andrade Date: Wed, 11 Dec 2024 23:44:04 +0100 Subject: [PATCH 1/2] Show a reply_count that matches what the user sees. Missing specs and polishing. --- app/controllers/api/v1/statuses_controller.rb | 31 +--- app/lib/activitypub/tag_manager.rb | 2 + app/models/context.rb | 5 - app/models/status.rb | 13 +- app/models/status_tree.rb | 144 ++++++++++++++++++ ...izer.rb => status_tree_node_serializer.rb} | 2 +- 6 files changed, 151 insertions(+), 46 deletions(-) delete mode 100644 app/models/context.rb create mode 100644 app/models/status_tree.rb rename app/serializers/rest/{context_serializer.rb => status_tree_node_serializer.rb} (71%) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 19cc71ae58..54b60ca265 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -14,16 +14,6 @@ class Api::V1::StatusesController < Api::BaseController override_rate_limit_headers :create, family: :statuses override_rate_limit_headers :update, family: :statuses - # This API was originally unlimited, pagination cannot be introduced without - # breaking backwards-compatibility. Arbitrarily high number to cover most - # conversations as quasi-unlimited, it would be too much work to render more - # than this anyway - CONTEXT_LIMIT = 4_096 - - # This remains expensive and we don't want to show everything to logged-out users - ANCESTORS_LIMIT = 40 - DESCENDANTS_LIMIT = 60 - DESCENDANTS_DEPTH_LIMIT = 20 def index @statuses = preload_collection(@statuses, Status) @@ -39,25 +29,10 @@ class Api::V1::StatusesController < Api::BaseController def context cache_if_unauthenticated! - ancestors_limit = CONTEXT_LIMIT - descendants_limit = CONTEXT_LIMIT - descendants_depth_limit = nil + @status_tree = StatusTree.new(status: @status, account: current_account) + @status_node = @status_tree.find_node(@status.id) - if current_account.nil? - ancestors_limit = ANCESTORS_LIMIT - descendants_limit = DESCENDANTS_LIMIT - descendants_depth_limit = DESCENDANTS_DEPTH_LIMIT - end - - ancestors_results = @status.in_reply_to_id.nil? ? [] : @status.ancestors(ancestors_limit, current_account) - descendants_results = @status.descendants(descendants_limit, current_account, descendants_depth_limit) - loaded_ancestors = preload_collection(ancestors_results, Status) - loaded_descendants = preload_collection(descendants_results, Status) - - @context = Context.new(ancestors: loaded_ancestors, descendants: loaded_descendants) - statuses = [@status] + @context.ancestors + @context.descendants - - render json: @context, serializer: REST::ContextSerializer, relationships: StatusRelationshipsPresenter.new(statuses, current_user&.account_id) + render json: @status_node, serializer: REST::StatusTreeNodeSerializer, relationships: StatusRelationshipsPresenter.new(@status_tree.flatten, current_user&.account_id) end def create diff --git a/app/lib/activitypub/tag_manager.rb b/app/lib/activitypub/tag_manager.rb index 23b44be372..97982b69e2 100644 --- a/app/lib/activitypub/tag_manager.rb +++ b/app/lib/activitypub/tag_manager.rb @@ -37,6 +37,8 @@ class ActivityPub::TagManager return target.uri if target.respond_to?(:local?) && !target.local? case target.object_type + when :status + return activity_account_status_url(target.account, target) if target.reblog? when :person target.instance_actor? ? instance_actor_url : account_url(target) when :note, :comment, :activity diff --git a/app/models/context.rb b/app/models/context.rb deleted file mode 100644 index cc667999ed..0000000000 --- a/app/models/context.rb +++ /dev/null @@ -1,5 +0,0 @@ -# frozen_string_literal: true - -class Context < ActiveModelSerializers::Model - attributes :ancestors, :descendants -end diff --git a/app/models/status.rb b/app/models/status.rb index 5a81b00773..3f374da686 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -292,18 +292,6 @@ class Status < ApplicationRecord end.take(MEDIA_ATTACHMENTS_LIMIT) end - def replies_count - status_stat&.replies_count || 0 - end - - def reblogs_count - status_stat&.reblogs_count || 0 - end - - def favourites_count - status_stat&.favourites_count || 0 - end - # Reblogs count received from an external instance def untrusted_reblogs_count status_stat&.untrusted_reblogs_count unless local? @@ -393,6 +381,7 @@ class Status < ApplicationRecord def status_stat super || build_status_stat end + delegate :replies_count, :reblogs_count, :favourites_count, to: :status_stat def discard_with_reblogs discard_time = Time.current diff --git a/app/models/status_tree.rb b/app/models/status_tree.rb new file mode 100644 index 0000000000..31586075ff --- /dev/null +++ b/app/models/status_tree.rb @@ -0,0 +1,144 @@ +class StatusTree < ActiveModelSerializers::Model + include PreloadingConcern + + # This API was originally unlimited, pagination cannot be introduced without + # breaking backwards-compatibility. Arbitrarily high number to cover most + # conversations as quasi-unlimited, it would be too much work to render more + # than this anyway + MAX_COUNT = 4_096 + + # This remains expensive and we don't want to show everything to logged-out users + ANCESTORS_MAX_COUNT = 40 + DESCENDANTS_MAX_COUNT = 60 + DESCENDANTS_MAX_DEPTH = 20 + + attributes :status, :account, :tree + + class Node < ActiveModelSerializers::Model + attributes :status, :tree + + delegate_missing_to :status + + delegate :id, to: :status + + def object_type = :status + + def ancestors + tree.ancestors_for(id) + end + + def descendants + tree.descendants_for(id) + end + + def children + tree.children_for(id) + end + + def replies_count + children.size + end + + def ==(other) + other.class.in?([Node, Status]) && id == other.id + end + + def inspect + "#" + end + end + + def tree + @tree ||= begin + ancestors = preload_collection(status.in_reply_to_id.nil? ? [] : status.ancestors(ancestors_max_count, account), Status) + descendants = preload_collection(status.descendants(descendants_max_count, account, descendants_max_depth), Status) + all_nodes = (ancestors + [status] + descendants).map { |status| Node.new(status:, tree: self) } + build_tree_from(all_nodes) + end + end + + def subtree_for(id, subtree = tree) + subtree.each do |node, children| + return children if node.id == id + + found = subtree_for(id, children) + return found if found + end + nil + end + + def flatten + collect_descendants(tree) + end + + delegate :each, :flat_map, :keys, to: :tree + + def inspect + "#" + end + + def find_node(id, subtree = tree) + subtree.each do |node, children| + return node if node.id == id + + result = find_node(id, children) + return result if result + end + end + + def ancestors_for(id) + ancestors = [] + node = find_node(id) + parent_id = node.in_reply_to_id + + while parent_id + parent_node = find_node(parent_id) + break unless parent_node + ancestors << parent_node + parent_id = parent_node.in_reply_to_id + end + + ancestors.reverse + end + + def descendants_for(id) + subtree = subtree_for(id) + return [] unless subtree + + collect_descendants(subtree) + end + + def children_for(id) + subtree = subtree_for(id) + + subtree.keys + end + + private + + def build_tree_from(nodes, parent_id = nil) + grouped_nodes = nodes.group_by(&:in_reply_to_id) + + (grouped_nodes[parent_id] || []).each_with_object({}) do |node, tree| + tree[node] = build_tree_from(nodes - [node], node.id) + end + end + + def descendants_max_depth + account.nil? ? DESCENDANTS_MAX_DEPTH : nil + end + + def descendants_max_count + account.nil? ? DESCENDANTS_MAX_COUNT : MAX_COUNT + end + + def ancestors_max_count + account.nil? ? ANCESTORS_MAX_COUNT : MAX_COUNT + end + + def collect_descendants(subtree) + subtree.flat_map do |node, children| + [node] + collect_descendants(children) + end + end +end diff --git a/app/serializers/rest/context_serializer.rb b/app/serializers/rest/status_tree_node_serializer.rb similarity index 71% rename from app/serializers/rest/context_serializer.rb rename to app/serializers/rest/status_tree_node_serializer.rb index 44515c85d7..63b7b17b78 100644 --- a/app/serializers/rest/context_serializer.rb +++ b/app/serializers/rest/status_tree_node_serializer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class REST::ContextSerializer < ActiveModel::Serializer +class REST::StatusTreeNodeSerializer < ActiveModel::Serializer has_many :ancestors, serializer: REST::StatusSerializer has_many :descendants, serializer: REST::StatusSerializer end From 26ac75dac44b3a21863a8737929fc06145bd9ff2 Mon Sep 17 00:00:00 2001 From: Guilherme Andrade Date: Tue, 17 Dec 2024 12:34:14 +0000 Subject: [PATCH 2/2] Minimize amount of changes and maximize the number of affected features by moving the replies_count patch to the serializer directly --- app/controllers/api/v1/statuses_controller.rb | 31 +++++++- app/lib/activitypub/tag_manager.rb | 2 - app/models/context.rb | 5 ++ app/models/status_tree.rb | 41 ++++------- ...de_serializer.rb => context_serializer.rb} | 2 +- app/serializers/rest/status_serializer.rb | 4 + .../rest/status_serializer_spec.rb | 73 +++++++++++++++++++ 7 files changed, 127 insertions(+), 31 deletions(-) create mode 100644 app/models/context.rb rename app/serializers/rest/{status_tree_node_serializer.rb => context_serializer.rb} (71%) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 54b60ca265..19cc71ae58 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -14,6 +14,16 @@ class Api::V1::StatusesController < Api::BaseController override_rate_limit_headers :create, family: :statuses override_rate_limit_headers :update, family: :statuses + # This API was originally unlimited, pagination cannot be introduced without + # breaking backwards-compatibility. Arbitrarily high number to cover most + # conversations as quasi-unlimited, it would be too much work to render more + # than this anyway + CONTEXT_LIMIT = 4_096 + + # This remains expensive and we don't want to show everything to logged-out users + ANCESTORS_LIMIT = 40 + DESCENDANTS_LIMIT = 60 + DESCENDANTS_DEPTH_LIMIT = 20 def index @statuses = preload_collection(@statuses, Status) @@ -29,10 +39,25 @@ class Api::V1::StatusesController < Api::BaseController def context cache_if_unauthenticated! - @status_tree = StatusTree.new(status: @status, account: current_account) - @status_node = @status_tree.find_node(@status.id) + ancestors_limit = CONTEXT_LIMIT + descendants_limit = CONTEXT_LIMIT + descendants_depth_limit = nil - render json: @status_node, serializer: REST::StatusTreeNodeSerializer, relationships: StatusRelationshipsPresenter.new(@status_tree.flatten, current_user&.account_id) + if current_account.nil? + ancestors_limit = ANCESTORS_LIMIT + descendants_limit = DESCENDANTS_LIMIT + descendants_depth_limit = DESCENDANTS_DEPTH_LIMIT + end + + ancestors_results = @status.in_reply_to_id.nil? ? [] : @status.ancestors(ancestors_limit, current_account) + descendants_results = @status.descendants(descendants_limit, current_account, descendants_depth_limit) + loaded_ancestors = preload_collection(ancestors_results, Status) + loaded_descendants = preload_collection(descendants_results, Status) + + @context = Context.new(ancestors: loaded_ancestors, descendants: loaded_descendants) + statuses = [@status] + @context.ancestors + @context.descendants + + render json: @context, serializer: REST::ContextSerializer, relationships: StatusRelationshipsPresenter.new(statuses, current_user&.account_id) end def create diff --git a/app/lib/activitypub/tag_manager.rb b/app/lib/activitypub/tag_manager.rb index 97982b69e2..23b44be372 100644 --- a/app/lib/activitypub/tag_manager.rb +++ b/app/lib/activitypub/tag_manager.rb @@ -37,8 +37,6 @@ class ActivityPub::TagManager return target.uri if target.respond_to?(:local?) && !target.local? case target.object_type - when :status - return activity_account_status_url(target.account, target) if target.reblog? when :person target.instance_actor? ? instance_actor_url : account_url(target) when :note, :comment, :activity diff --git a/app/models/context.rb b/app/models/context.rb new file mode 100644 index 0000000000..cc667999ed --- /dev/null +++ b/app/models/context.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class Context < ActiveModelSerializers::Model + attributes :ancestors, :descendants +end diff --git a/app/models/status_tree.rb b/app/models/status_tree.rb index 31586075ff..7f79709fad 100644 --- a/app/models/status_tree.rb +++ b/app/models/status_tree.rb @@ -1,26 +1,17 @@ class StatusTree < ActiveModelSerializers::Model include PreloadingConcern - # This API was originally unlimited, pagination cannot be introduced without - # breaking backwards-compatibility. Arbitrarily high number to cover most - # conversations as quasi-unlimited, it would be too much work to render more - # than this anyway MAX_COUNT = 4_096 - # This remains expensive and we don't want to show everything to logged-out users - ANCESTORS_MAX_COUNT = 40 - DESCENDANTS_MAX_COUNT = 60 - DESCENDANTS_MAX_DEPTH = 20 - attributes :status, :account, :tree class Node < ActiveModelSerializers::Model attributes :status, :tree - delegate_missing_to :status - delegate :id, to: :status + delegate_missing_to :status + def object_type = :status def ancestors @@ -35,16 +26,12 @@ class StatusTree < ActiveModelSerializers::Model tree.children_for(id) end - def replies_count - children.size - end - def ==(other) other.class.in?([Node, Status]) && id == other.id end def inspect - "#" + "#" end end @@ -77,6 +64,10 @@ class StatusTree < ActiveModelSerializers::Model "#" end + def status_node + find_node(status.id) + end + def find_node(id, subtree = tree) subtree.each do |node, children| return node if node.id == id @@ -89,13 +80,13 @@ class StatusTree < ActiveModelSerializers::Model def ancestors_for(id) ancestors = [] node = find_node(id) - parent_id = node.in_reply_to_id + in_reply_to_id = node.in_reply_to_id - while parent_id - parent_node = find_node(parent_id) + while in_reply_to_id + parent_node = find_node(in_reply_to_id) break unless parent_node ancestors << parent_node - parent_id = parent_node.in_reply_to_id + in_reply_to_id = parent_node.in_reply_to_id end ancestors.reverse @@ -116,24 +107,24 @@ class StatusTree < ActiveModelSerializers::Model private - def build_tree_from(nodes, parent_id = nil) + def build_tree_from(nodes, in_reply_to_id = nil) grouped_nodes = nodes.group_by(&:in_reply_to_id) - (grouped_nodes[parent_id] || []).each_with_object({}) do |node, tree| + (grouped_nodes[in_reply_to_id] || []).each_with_object({}) do |node, tree| tree[node] = build_tree_from(nodes - [node], node.id) end end def descendants_max_depth - account.nil? ? DESCENDANTS_MAX_DEPTH : nil + nil end def descendants_max_count - account.nil? ? DESCENDANTS_MAX_COUNT : MAX_COUNT + MAX_COUNT end def ancestors_max_count - account.nil? ? ANCESTORS_MAX_COUNT : MAX_COUNT + MAX_COUNT end def collect_descendants(subtree) diff --git a/app/serializers/rest/status_tree_node_serializer.rb b/app/serializers/rest/context_serializer.rb similarity index 71% rename from app/serializers/rest/status_tree_node_serializer.rb rename to app/serializers/rest/context_serializer.rb index 63b7b17b78..44515c85d7 100644 --- a/app/serializers/rest/status_tree_node_serializer.rb +++ b/app/serializers/rest/context_serializer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class REST::StatusTreeNodeSerializer < ActiveModel::Serializer +class REST::ContextSerializer < ActiveModel::Serializer has_many :ancestors, serializer: REST::StatusSerializer has_many :descendants, serializer: REST::StatusSerializer end diff --git a/app/serializers/rest/status_serializer.rb b/app/serializers/rest/status_serializer.rb index e108c789c7..f00ad2eba1 100644 --- a/app/serializers/rest/status_serializer.rb +++ b/app/serializers/rest/status_serializer.rb @@ -91,6 +91,10 @@ class REST::StatusSerializer < ActiveModel::Serializer object.untrusted_favourites_count || relationships&.attributes_map&.dig(object.id, :favourites_count) || object.favourites_count end + def replies_count + StatusTree.new(status: object, account: current_user&.account).status_node.children.size + end + def favourited if relationships relationships.favourites_map[object.id] || false diff --git a/spec/serializers/rest/status_serializer_spec.rb b/spec/serializers/rest/status_serializer_spec.rb index e96d1fbe67..e6bd59349c 100644 --- a/spec/serializers/rest/status_serializer_spec.rb +++ b/spec/serializers/rest/status_serializer_spec.rb @@ -52,4 +52,77 @@ RSpec.describe REST::StatusSerializer do end end end + + describe '#replies_count' do + let(:author) { alice } + let(:replier) { bob } + let!(:status) { Fabricate(:status, account: author, visibility: :public) } + + context 'when being presented to the account that posted the status' do + let(:current_user) { Fabricate(:user, account: author) } + + before do + Fabricate(:follow, account: replier, target_account: author) + Fabricate(:follow, account: author, target_account: replier) + end + + context 'when the status has follower-only replies' do + let(:reply) { Fabricate(:status, in_reply_to_id: status.id, account: replier, visibility: :private) } + + before do + reply + end + + it 'counts 1 reply' do + expect(subject['replies_count']).to eq(1) + end + + context 'when one of the replies has subsequent replies' do + before do + Fabricate(:status, in_reply_to_id: reply.id, account: author, visibility: :private) + end + + it 'does not count that reply' do + expect(subject['replies_count']).to eq 1 + end + end + end + end + + context 'when being presented to a different account' do + let(:current_user) { Fabricate(:user) } + + context 'when the status has follower-only replies from an unfollowed account' do + before do + Fabricate(:status, in_reply_to_id: status.id, account: replier, visibility: :direct) + end + + it 'counts 0 replies' do + expect(subject['replies_count']).to be 0 + end + end + + context 'when the replies are public' do + before do + Fabricate(:status, in_reply_to_id: status.id, account: replier, visibility: :public) + end + + it 'counts 1 reply' do + expect(subject['replies_count']).to eq 1 + end + end + + context 'when there is one public reply and one private' do + before do + %i[direct public].each do |visibility| + Fabricate(:status, in_reply_to_id: status.id, account: replier, visibility: visibility) + end + end + + it 'counts 1 reply' do + expect(subject['replies_count']).to eq 1 + end + end + end + end end