From 86f6631d283423746b8fdf0a618f6e0abafea099 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 10 Nov 2022 22:30:00 +0100 Subject: [PATCH] Remove dead code and refactor status threading code (#20357) * Remove dead code * Remove unneeded/broken parameters and refactor descendant computation --- app/controllers/api/v1/statuses_controller.rb | 2 +- .../concerns/status_controller_concern.rb | 87 ------------------- app/controllers/statuses_controller.rb | 1 - .../concerns/status_threading_concern.rb | 21 ++--- 4 files changed, 9 insertions(+), 102 deletions(-) delete mode 100644 app/controllers/concerns/status_controller_concern.rb diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index b43b6f1a7..6290a1746 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -40,7 +40,7 @@ class Api::V1::StatusesController < Api::BaseController end ancestors_results = @status.in_reply_to_id.nil? ? [] : @status.ancestors(ancestors_limit, current_account) - descendants_results = @status.descendants(descendants_limit, current_account, nil, nil, descendants_depth_limit) + descendants_results = @status.descendants(descendants_limit, current_account, descendants_depth_limit) loaded_ancestors = cache_collection(ancestors_results, Status) loaded_descendants = cache_collection(descendants_results, Status) diff --git a/app/controllers/concerns/status_controller_concern.rb b/app/controllers/concerns/status_controller_concern.rb deleted file mode 100644 index 62a7cf508..000000000 --- a/app/controllers/concerns/status_controller_concern.rb +++ /dev/null @@ -1,87 +0,0 @@ -# frozen_string_literal: true - -module StatusControllerConcern - extend ActiveSupport::Concern - - ANCESTORS_LIMIT = 40 - DESCENDANTS_LIMIT = 60 - DESCENDANTS_DEPTH_LIMIT = 20 - - def create_descendant_thread(starting_depth, statuses) - depth = starting_depth + statuses.size - - if depth < DESCENDANTS_DEPTH_LIMIT - { - statuses: statuses, - starting_depth: starting_depth, - } - else - next_status = statuses.pop - - { - statuses: statuses, - starting_depth: starting_depth, - next_status: next_status, - } - end - end - - def set_ancestors - @ancestors = @status.reply? ? cache_collection(@status.ancestors(ANCESTORS_LIMIT, current_account), Status) : [] - @next_ancestor = @ancestors.size < ANCESTORS_LIMIT ? nil : @ancestors.shift - end - - def set_descendants - @max_descendant_thread_id = params[:max_descendant_thread_id]&.to_i - @since_descendant_thread_id = params[:since_descendant_thread_id]&.to_i - - descendants = cache_collection( - @status.descendants( - DESCENDANTS_LIMIT, - current_account, - @max_descendant_thread_id, - @since_descendant_thread_id, - DESCENDANTS_DEPTH_LIMIT - ), - Status - ) - - @descendant_threads = [] - - if descendants.present? - statuses = [descendants.first] - starting_depth = 0 - - descendants.drop(1).each_with_index do |descendant, index| - if descendants[index].id == descendant.in_reply_to_id - statuses << descendant - else - @descendant_threads << create_descendant_thread(starting_depth, statuses) - - # The thread is broken, assume it's a reply to the root status - starting_depth = 0 - - # ... unless we can find its ancestor in one of the already-processed threads - @descendant_threads.reverse_each do |descendant_thread| - statuses = descendant_thread[:statuses] - - index = statuses.find_index do |thread_status| - thread_status.id == descendant.in_reply_to_id - end - - if index.present? - starting_depth = descendant_thread[:starting_depth] + index + 1 - break - end - end - - statuses = [descendant] - end - end - - @descendant_threads << create_descendant_thread(starting_depth, statuses) - end - - @max_descendant_thread_id = @descendant_threads.pop[:statuses].first.id if descendants.size >= DESCENDANTS_LIMIT - end -end diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index bb4e5b01f..9eb7ad691 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -2,7 +2,6 @@ class StatusesController < ApplicationController include WebAppControllerConcern - include StatusControllerConcern include SignatureAuthentication include Authorization include AccountOwnedConcern diff --git a/app/models/concerns/status_threading_concern.rb b/app/models/concerns/status_threading_concern.rb index 5c04108e4..8b628beea 100644 --- a/app/models/concerns/status_threading_concern.rb +++ b/app/models/concerns/status_threading_concern.rb @@ -7,8 +7,8 @@ module StatusThreadingConcern find_statuses_from_tree_path(ancestor_ids(limit), account) end - def descendants(limit, account = nil, max_child_id = nil, since_child_id = nil, depth = nil) - find_statuses_from_tree_path(descendant_ids(limit, max_child_id, since_child_id, depth), account, promote: true) + def descendants(limit, account = nil, depth = nil) + find_statuses_from_tree_path(descendant_ids(limit, depth), account, promote: true) end def self_replies(limit) @@ -50,22 +50,17 @@ module StatusThreadingConcern SQL end - def descendant_ids(limit, max_child_id, since_child_id, depth) - descendant_statuses(limit, max_child_id, since_child_id, depth).pluck(:id) - end - - def descendant_statuses(limit, max_child_id, since_child_id, depth) + def descendant_ids(limit, depth) # use limit + 1 and depth + 1 because 'self' is included depth += 1 if depth.present? limit += 1 if limit.present? - descendants_with_self = Status.find_by_sql([<<-SQL.squish, id: id, limit: limit, max_child_id: max_child_id, since_child_id: since_child_id, depth: depth]) - WITH RECURSIVE search_tree(id, path) - AS ( + descendants_with_self = Status.find_by_sql([<<-SQL.squish, id: id, limit: limit, depth: depth]) + WITH RECURSIVE search_tree(id, path) AS ( SELECT id, ARRAY[id] FROM statuses - WHERE id = :id AND COALESCE(id < :max_child_id, TRUE) AND COALESCE(id > :since_child_id, TRUE) - UNION ALL + WHERE id = :id + UNION ALL SELECT statuses.id, path || statuses.id FROM search_tree JOIN statuses ON statuses.in_reply_to_id = search_tree.id @@ -77,7 +72,7 @@ module StatusThreadingConcern LIMIT :limit SQL - descendants_with_self - [self] + descendants_with_self.pluck(:id) - [id] end def find_statuses_from_tree_path(ids, account, promote: false)