diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 055ae9d3e7c..635d121fe94 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -336,26 +336,17 @@ class CategoriesController < ApplicationController end def hierarchical_search - term = params[:term].to_s.strip - page = [1, params[:page].to_i].max - only_ids = params[:only].map(&:to_i) if params[:only].present? - except_ids = params[:except].map(&:to_i) if params[:except].present? - - categories = - CategoryHierarchicalSearch.call( - guardian: guardian, - params: { - term:, - only_ids:, - except_ids:, - limit: MAX_CATEGORIES_LIMIT, - offset: (page - 1) * MAX_CATEGORIES_LIMIT, - }, - ).categories - - response = { categories: serialize_data(categories, SiteCategorySerializer, scope: guardian) } - - render_json_dump(response) + Category::HierarchicalSearch.call(service_params) do + on_success do |categories:| + render_json_dump( + categories: serialize_data(categories, SiteCategorySerializer, scope: guardian), + ) + end + on_failed_contract do |contract| + render json: failed_json.merge(errors: contract.errors.full_messages), status: :bad_request + end + on_failure { render json: failed_json, status: :unprocessable_entity } + end end def search diff --git a/app/services/category/action/eager_load_associations.rb b/app/services/category/action/eager_load_associations.rb new file mode 100644 index 00000000000..e1ebdb001df --- /dev/null +++ b/app/services/category/action/eager_load_associations.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +class Category::Action::EagerLoadAssociations < Service::ActionBase + option :categories, [] + option :guardian + + ASSOCIATIONS = [ + :uploaded_logo, + :uploaded_logo_dark, + :uploaded_background, + :uploaded_background_dark, + :tags, + :tag_groups, + :form_templates, + { category_required_tag_groups: :tag_group }, + ].freeze + + def call + return if categories.blank? + preload_associations + preload_custom_fields + preload_user_fields + end + + private + + def preload_associations + ActiveRecord::Associations::Preloader.new(records: categories, associations: ASSOCIATIONS).call + end + + def preload_custom_fields + return if Site.preloaded_category_custom_fields.blank? + Category.preload_custom_fields(categories, Site.preloaded_category_custom_fields) + end + + def preload_user_fields + Category.preload_user_fields!(guardian, categories) + end +end diff --git a/app/services/category/hierarchical_search.rb b/app/services/category/hierarchical_search.rb new file mode 100644 index 00000000000..2b334803ab2 --- /dev/null +++ b/app/services/category/hierarchical_search.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class Category::HierarchicalSearch + include Service::Base + + params do + attribute :term, :string, default: "" + attribute :only, :array, default: [], compact_blank: true + attribute :except, :array, default: [], compact_blank: true + attribute :page, :integer, default: 1 + + validates :page, numericality: { greater_than: 0 } + + after_validation { self.term = term.to_s.strip } + + def limit = CategoriesController::MAX_CATEGORIES_LIMIT + def offset = (page - 1) * limit + end + + model :categories, optional: true + step :eager_load_associations + + private + + def fetch_categories(guardian:, params:) + Category::Query::HierarchicalSearch.new(guardian:, params:).call + end + + def eager_load_associations(guardian:, categories:) + Category::Action::EagerLoadAssociations.call(categories:, guardian:) + end +end diff --git a/app/services/category/query/hierarchical_search.rb b/app/services/category/query/hierarchical_search.rb new file mode 100644 index 00000000000..a9f634a8b38 --- /dev/null +++ b/app/services/category/query/hierarchical_search.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +class Category::Query::HierarchicalSearch + def initialize(guardian:, params:) + @guardian = guardian + @params = params + end + + def call + build_relation.tap { prepend_allowed_categories_cte(_1) }.to_a + end + + private + + attr_reader :guardian, :params + + def build_relation + Category + .with(matched: matched_scope) + .with_recursive(matched_with_ancestors_cte) + .with_recursive(category_tree_cte) + .joins("INNER JOIN category_tree ct ON ct.id = categories.id") + .joins("INNER JOIN (SELECT DISTINCT id FROM matched_with_ancestors) a ON a.id = ct.id") + .order("ct.name_path, ct.id") + .limit(params.limit) + .offset(params.offset) + end + + # Prepends the allowed_categories CTE with NOT MATERIALIZED hint. + # Rails' .with() doesn't support materialization hints, so we manipulate Arel directly. + # NOT MATERIALIZED prevents PostgreSQL from materializing this CTE, which significantly + # improves performance on sites with many categories. + def prepend_allowed_categories_cte(relation) + cte = + Arel::Nodes::Cte.new( + Arel.sql("allowed_categories"), + allowed_categories_arel, + materialized: false, + ) + existing_ctes = relation.arel.ast.with&.children || [] + relation.arel.with(:recursive, [cte] + existing_ctes) + end + + def allowed_categories_arel + Category + .secured(guardian) + .where.not(id: SiteSetting.uncategorized_category_id) + .select(:id, :name, :parent_category_id) + .arel + end + + def matched_scope + scope = Category.from("allowed_categories").select(:id) + scope = scope.where(term_condition) if params.term.present? + scope = scope.where("id IN (?)", params.only) if params.only.present? + scope = scope.where("id NOT IN (?)", params.except) if params.except.present? + scope + end + + def quoted_term + @quoted_term ||= Category.normalize_sql(Category.connection.quote(params.term.downcase)) + end + + def term_condition + Arel.sql(<<~SQL.squish) + ( + starts_with(#{Category.normalize_sql("name")}, #{quoted_term}) + OR COALESCE( + ( + SELECT BOOL_AND(position(pattern IN #{Category.normalize_sql("allowed_categories.name")}) <> 0) + FROM unnest(regexp_split_to_array(#{quoted_term}, '\\s+')) AS pattern + ), + true + ) + ) + SQL + end + + def matched_with_ancestors_cte + { matched_with_ancestors: [Arel.sql(<<~SQL), Arel.sql(<<~SQL)] } + SELECT c.id, c.parent_category_id + FROM allowed_categories c + JOIN matched m ON m.id = c.id + SQL + SELECT p.id, p.parent_category_id + FROM allowed_categories p + JOIN matched_with_ancestors a ON a.parent_category_id = p.id + SQL + end + + def category_tree_cte + { category_tree: [Arel.sql(<<~SQL), Arel.sql(<<~SQL)] } + SELECT + c.id, + c.parent_category_id, + c.name, + ARRAY[lower(c.name)]::text[] AS name_path, + 0 AS depth + FROM allowed_categories c + WHERE c.parent_category_id IS NULL + SQL + SELECT + c.id, + c.parent_category_id, + c.name, + ct.name_path || lower(c.name), + ct.depth + 1 + FROM allowed_categories c + JOIN category_tree ct ON c.parent_category_id = ct.id + SQL + end +end diff --git a/app/services/category_hierarchical_search.rb b/app/services/category_hierarchical_search.rb deleted file mode 100644 index 412eac1f965..00000000000 --- a/app/services/category_hierarchical_search.rb +++ /dev/null @@ -1,146 +0,0 @@ -# frozen_string_literal: true - -class CategoryHierarchicalSearch - include Service::Base - - params do - attribute :term, :string, default: "" - attribute :only_ids, :array, default: [], compact_blank: true - attribute :except_ids, :array, default: [], compact_blank: true - attribute :limit, :integer - attribute :offset, :integer - - after_validation { self.term = term.to_s.strip } - end - - model :categories, optional: true - step :eager_load_associations - - private - - def fetch_categories(guardian:, params:) - query_params = { offset: params.offset, limit: params.limit, term: params.term } - query_params[:only_ids] = params.only_ids if params.only_ids.present? - query_params[:except_ids] = params.except_ids if params.except_ids.present? - - allowed_categories_sql = - Category - .secured(guardian) - .select(:id, :name, :parent_category_id) - .where("id <> ?", SiteSetting.uncategorized_category_id) - .to_sql - - matches_sql = - if query_params[:term].present? - <<~SQL - ( - starts_with(#{Category.normalize_sql("name")}, #{Category.normalize_sql(":term")}) - OR COALESCE( - ( - SELECT BOOL_AND(position(pattern IN #{Category.normalize_sql("allowed_categories.name")}) <> 0) - FROM unnest(regexp_split_to_array(#{Category.normalize_sql(":term")}, '\\s+')) AS pattern - ), - true - ) - ) - SQL - else - "TRUE" - end - - only_ids_sql = - if query_params[:only_ids].present? - "AND allowed_categories.id IN (:only_ids)" - else - "" - end - - except_ids_sql = - if query_params[:except_ids].present? - "AND allowed_categories.id NOT IN (:except_ids)" - else - "" - end - - # Note that we are setting the `allowed_categories` CTE as `NOT MATERIALIZED` since materializing the CTE degrades - # performance of the query significantly on sites with a large number of rows in the categories table - sql = <<~SQL - WITH RECURSIVE - allowed_categories AS NOT MATERIALIZED ( - #{allowed_categories_sql} - ), - matched AS ( - SELECT id - FROM allowed_categories - WHERE - #{matches_sql} - #{only_ids_sql} - #{except_ids_sql} - ), - matched_with_ancestors AS ( - SELECT c.id, c.parent_category_id - FROM allowed_categories c - JOIN matched m ON m.id = c.id - - UNION ALL - - SELECT p.id, p.parent_category_id - FROM allowed_categories p - JOIN matched_with_ancestors a ON a.parent_category_id = p.id - ), - category_tree AS ( - SELECT - c.id, - c.parent_category_id, - c.name, - ARRAY[lower(c.name)]::text[] AS name_path, - 0 AS depth - FROM allowed_categories c - WHERE c.parent_category_id IS NULL - - UNION ALL - - SELECT - c.id, - c.parent_category_id, - c.name, - ct.name_path || lower(c.name), - ct.depth + 1 - FROM allowed_categories c - JOIN category_tree ct ON c.parent_category_id = ct.id - ) - SELECT - categories.* - FROM category_tree ct - INNER JOIN categories ON categories.id = ct.id - INNER JOIN (SELECT DISTINCT id FROM matched_with_ancestors) a ON a.id = ct.id - ORDER BY ct.name_path, ct.id - #{params.limit.present? ? "LIMIT :limit" : ""} - #{params.offset.present? ? "OFFSET :offset" : ""} - SQL - - Category.find_by_sql([sql, query_params]) - end - - def eager_load_associations - ActiveRecord::Associations::Preloader.new( - records: context.categories, - associations: [ - :uploaded_logo, - :uploaded_logo_dark, - :uploaded_background, - :uploaded_background_dark, - :tags, - :tag_groups, - :form_templates, - { category_required_tag_groups: :tag_group }, - ], - ).call - - if Site.preloaded_category_custom_fields.present? - Category.preload_custom_fields(context.categories, Site.preloaded_category_custom_fields) - end - - Category.preload_user_fields!(context.guardian, context.categories) - end -end diff --git a/spec/services/category/action/eager_load_associations_spec.rb b/spec/services/category/action/eager_load_associations_spec.rb new file mode 100644 index 00000000000..b6535a14680 --- /dev/null +++ b/spec/services/category/action/eager_load_associations_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +RSpec.describe Category::Action::EagerLoadAssociations do + describe ".call" do + subject(:action) { described_class.call(categories:, guardian:) } + + fab!(:user) + fab!(:category) + + let(:guardian) { Guardian.new(user) } + let(:categories) { [category] } + + it "preloads associations without errors" do + expect { action }.not_to raise_error + end + + context "when categories is empty" do + let(:categories) { [] } + + it "handles empty categories" do + expect { action }.not_to raise_error + end + end + + context "with custom fields configured" do + before do + allow(Site).to receive(:preloaded_category_custom_fields).and_return(%w[custom_field]) + allow(Category).to receive(:preload_custom_fields) + end + + it "preloads custom fields" do + action + expect(Category).to have_received(:preload_custom_fields).with(categories, %w[custom_field]) + end + end + end +end diff --git a/spec/services/category/hierarchical_search_spec.rb b/spec/services/category/hierarchical_search_spec.rb new file mode 100644 index 00000000000..40dff908629 --- /dev/null +++ b/spec/services/category/hierarchical_search_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +RSpec.describe Category::HierarchicalSearch do + describe described_class::Contract, type: :model do + subject(:contract) { described_class.new(page:) } + + let(:page) { 1 } + + it { is_expected.to allow_values(1, 100).for(:page) } + it { is_expected.not_to allow_values(0, -1).for(:page) } + + describe "#limit" do + it { expect(contract.limit).to eq(CategoriesController::MAX_CATEGORIES_LIMIT) } + end + + describe "#offset" do + let(:page) { 3 } + + it { expect(contract.offset).to eq(2 * CategoriesController::MAX_CATEGORIES_LIMIT) } + end + end + + describe ".call" do + subject(:result) { described_class.call(guardian:, params:) } + + fab!(:user) + fab!(:category) + + let(:guardian) { Guardian.new(user) } + let(:params) { { term: "test" } } + let(:categories) { [category] } + let(:query_instance) { instance_double(Category::Query::HierarchicalSearch, call: categories) } + + context "with invalid data" do + let(:params) { { page: -1 } } + + it { is_expected.to fail_a_contract } + end + + context "when everything's ok" do + before do + allow(Category::Query::HierarchicalSearch).to receive(:new).and_return(query_instance) + allow(Category::Action::EagerLoadAssociations).to receive(:call) + end + + it { is_expected.to run_successfully } + + it "returns categories from hierarchical search" do + expect(result.categories).to eq(categories) + expect(Category::Query::HierarchicalSearch).to have_received(:new).with( + guardian:, + params: result[:params], + ) + end + + it "eager loads associations" do + result + expect(Category::Action::EagerLoadAssociations).to have_received(:call).with( + categories:, + guardian:, + ) + end + end + end +end diff --git a/spec/services/category/query/hierarchical_search_spec.rb b/spec/services/category/query/hierarchical_search_spec.rb new file mode 100644 index 00000000000..e555cfbbadf --- /dev/null +++ b/spec/services/category/query/hierarchical_search_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +RSpec.describe Category::Query::HierarchicalSearch do + # before_all: allows fab! to create 3-level nested categories + # before: ensures setting persists (site settings reset between tests) + before_all { SiteSetting.max_category_nesting = 3 } + before { SiteSetting.max_category_nesting = 3 } + + fab!(:user) + fab!(:parent) { Fabricate(:category, name: "Parent") } + fab!(:child) { Fabricate(:category, name: "Child", parent_category: parent) } + fab!(:grandchild) { Fabricate(:category, name: "Grandchild Match", parent_category: child) } + + let(:guardian) { Guardian.new(user) } + let(:term) { "" } + let(:only) { [] } + let(:except) { [] } + let(:page) { 1 } + let(:params) { Category::HierarchicalSearch::Contract.new(term:, only:, except:, page:) } + + describe "#call" do + subject(:result) { described_class.new(guardian:, params:).call } + + context "when searching by term" do + let(:term) { "match" } + + it "returns matching categories with ancestors in hierarchical order" do + expect(result).to eq([parent, child, grandchild]) + end + end + + context "when filtering by only" do + let(:only) { [grandchild.id] } + + it "returns specified categories with ancestors" do + expect(result).to eq([parent, child, grandchild]) + end + end + + context "when filtering by except" do + let(:except) { [grandchild.id] } + + it "excludes specified categories" do + expect(result).to eq([parent, child]) + end + end + + context "when paginating" do + let(:page) { 2 } + + it "offsets results by page" do + # Page 2 with limit 25 would offset by 25, returning nothing for our small dataset + expect(result).to be_empty + end + end + + context "when guardian cannot see category" do + fab!(:restricted_group, :group) + fab!(:restricted_category) do + Fabricate(:category, name: "Restricted").tap do |c| + c.set_permissions(restricted_group => :full) + c.save! + end + end + + let(:term) { "restricted" } + + it "excludes restricted categories" do + expect(result).to be_empty + end + end + + it "excludes uncategorized category" do + expect(result).not_to include(have_attributes(id: SiteSetting.uncategorized_category_id)) + end + end +end diff --git a/spec/services/category_hierarchical_search_spec.rb b/spec/services/category_hierarchical_search_spec.rb deleted file mode 100644 index 98aba3b28a4..00000000000 --- a/spec/services/category_hierarchical_search_spec.rb +++ /dev/null @@ -1,200 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe CategoryHierarchicalSearch do - before_all { SiteSetting.max_category_nesting = 3 } - - fab!(:user) - fab!(:parent_2) { Fabricate(:category, name: "Parent 2") } - fab!(:parent_1) { Fabricate(:category, name: "Parent 1") } - - fab!(:parent_1_sub_category_1) do - Fabricate(:category, name: "Parent 1 Sub Category 1", parent_category: parent_1) - end - - fab!(:parent_1_sub_category_2) do - Fabricate(:category, name: "Parent 1 Sub Category 2", parent_category: parent_1) - end - - fab!(:parent_2_sub_category_1) do - Fabricate(:category, name: "Parent 2 Sub Category 1", parent_category: parent_2) - end - - fab!(:parent_2_sub_category_2) do - Fabricate(:category, name: "Parent 2 Sub Category 2", parent_category: parent_2) - end - - fab!(:parent_1_sub_category_1_sub_sub_category_1) do - Fabricate( - :category, - name: "Parent 1 Sub Category 1 Sub Sub Category 1 Match", - parent_category: parent_1_sub_category_1, - ) - end - - fab!(:parent_1_sub_category_1_sub_sub_category_2) do - Fabricate( - :category, - name: "Parent 1 Sub Category 1 Sub Sub Category 2", - parent_category: parent_1_sub_category_1, - ) - end - - fab!(:parent_1_sub_category_2_sub_sub_category_1) do - Fabricate( - :category, - name: "Parent 1 Sub Category 2 Sub Sub Category 1", - parent_category: parent_1_sub_category_2, - ) - end - - fab!(:parent_1_sub_category_2_sub_sub_category_2) do - Fabricate( - :category, - name: "Parent 1 Sub Category 2 Sub Sub Category 2", - parent_category: parent_1_sub_category_2, - ) - end - - fab!(:parent_2_sub_category_1_sub_sub_category_1) do - Fabricate( - :category, - name: "Parent 2 Sub Category 1 Sub Sub Category 1", - parent_category: parent_2_sub_category_1, - ) - end - - fab!(:parent_2_sub_category_1_sub_sub_category_2) do - Fabricate( - :category, - name: "Parent 2 Sub Category 1 Sub Sub Category 2", - parent_category: parent_2_sub_category_1, - ) - end - - fab!(:parent_2_sub_category_2_sub_sub_category_1) do - Fabricate( - :category, - name: "Parent 2 Sub Category 2 Sub Sub Category 1", - parent_category: parent_2_sub_category_2, - ) - end - - fab!(:parent_2_sub_category_2_sub_sub_category_2) do - Fabricate( - :category, - name: "Parent 2 Sub Category 2 Sub Sub Category 2 MATCH", - parent_category: parent_2_sub_category_2, - ) - end - - it "matches categories with accented names using unaccented search term" do - Fabricate(:category, name: "Éditions spéciales") - - context = described_class.call(guardian: Guardian.new, params: { term: "editions speciales" }) - - expect(context).to be_success - expect(context.categories.map(&:name)).to include("Éditions spéciales") - end - - it "returns categories with their ancestors that match the terms param in a hierarchical order" do - context = described_class.call(guardian: Guardian.new, params: { term: "match" }) - - expect(context).to be_success - - expect(context.categories.map(&:name)).to eq( - [ - parent_1.name, - parent_1_sub_category_1.name, - parent_1_sub_category_1_sub_sub_category_1.name, - parent_2.name, - parent_2_sub_category_2.name, - parent_2_sub_category_2_sub_sub_category_2.name, - ], - ) - end - - it "returns categories with their ancestors that have ids which are included in the only_ids param in a hierarchical order" do - context = - described_class.call( - guardian: Guardian.new, - params: { - only_ids: [ - parent_1_sub_category_1_sub_sub_category_1.id, - parent_2_sub_category_2_sub_sub_category_2.id, - ], - }, - ) - - expect(context).to be_success - - expect(context.categories.map(&:name)).to eq( - [ - parent_1.name, - parent_1_sub_category_1.name, - parent_1_sub_category_1_sub_sub_category_1.name, - parent_2.name, - parent_2_sub_category_2.name, - parent_2_sub_category_2_sub_sub_category_2.name, - ], - ) - end - - it "returns categories with their ancestors that have ids which is not included in the except_ids param in a hierarchical order" do - context = - described_class.call( - guardian: Guardian.new, - params: { - except_ids: [ - parent_1_sub_category_2_sub_sub_category_1.id, - parent_1_sub_category_2_sub_sub_category_2.id, - parent_2_sub_category_1_sub_sub_category_1.id, - parent_2_sub_category_1_sub_sub_category_2.id, - ], - }, - ) - - expect(context).to be_success - - expect(context.categories.map(&:name)).to eq( - [ - parent_1.name, - parent_1_sub_category_1.name, - parent_1_sub_category_1_sub_sub_category_1.name, - parent_1_sub_category_1_sub_sub_category_2.name, - parent_1_sub_category_2.name, - parent_2.name, - parent_2_sub_category_1.name, - parent_2_sub_category_2.name, - parent_2_sub_category_2_sub_sub_category_1.name, - parent_2_sub_category_2_sub_sub_category_2.name, - ], - ) - end - - it "excludes categories that the guardian cannot see" do - restricted_group = Fabricate(:group) - restricted_parent_category = Fabricate(:category, name: "Restricted Parent") - restricted_parent_category.set_permissions(restricted_group => :full) - restricted_parent_category.save! - - context = described_class.call(guardian: Guardian.new, params: { term: "restricted" }) - - expect(context).to be_success - expect(context.categories).to be_empty - end - - it "applies a limit and offset" do - context = described_class.call(guardian: Guardian.new, params: { limit: 2 }) - - expect(context.categories.map(&:name)).to eq([parent_1.name, parent_1_sub_category_1.name]) - - context = described_class.call(guardian: Guardian.new, params: { limit: 2, offset: 2 }) - - expect(context.categories.map(&:name)).to eq( - [ - parent_1_sub_category_1_sub_sub_category_1.name, - parent_1_sub_category_1_sub_sub_category_2.name, - ], - ) - end -end