DEV: Refactor category hierarchical search (#37609)

The monolithic `CategoryHierarchicalSearch` service mixed query
building, eager loading, and pagination logic in a single class. The
query was a large raw SQL string built through conditional string
concatenation. Fragments like `#{matches_sql}`, `#{only_ids_sql}`,
`#{except_ids_sql}` were stitched together, with LIMIT/OFFSET appended
via ternary interpolation and named placeholders passed through a
manually assembled hash. This made the query fragile and hard to follow.

Break it into focused, single-responsibility classes under the
`Category::` namespace:

- `Category::HierarchicalSearch` — service orchestrator using
`Service::Base`, with a contract that owns pagination logic (page
validation, limit/offset computation)
- `Category::Query::HierarchicalSearch` — query object that uses
ActiveRecord's interface where it naturally fits (`.where()` with
parameter binding, `.limit()`, `.offset()`, `.with()`, `.joins()`) and
isolates the genuinely complex SQL (recursive CTEs, term matching) into
small named methods rather than a monolithic heredoc
- `Category::Action::EagerLoadAssociations` — extracted eager loading
into a reusable `Service::ActionBase`

The controller is simplified to a single
`Category::HierarchicalSearch.call(service_params)` call with proper
`on_success` / `on_failed_contract` / `on_failure` handling, replacing
manual param transformation and direct result access.

Specs are rewritten to test each class in isolation: the service spec
stubs its collaborators to verify orchestration, the query spec
exercises actual SQL behavior, and the action spec verifies preloading.

Service structure and spec patterns follow the [Discourse service object
guidelines](https://meta.discourse.org/t/using-service-objects-in-discourse/333641)
and the [RSpec Style Guide](https://rspec.rubystyle.guide/).
This commit is contained in:
Loïc Guitaut
2026-02-13 09:43:56 +01:00
committed by GitHub
parent 0410416c33
commit bcf33a2901
9 changed files with 373 additions and 366 deletions
+11 -20
View File
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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