mirror of
https://github.com/discourse/discourse.git
synced 2026-09-05 04:40:41 -05:00
FIX: stop delegating AI search to filter (#36968)
We added delegation to handle cases such as order:latest, to resolve this we now added a param to Search.rb which allows it to perform searches with no filter, this allows for order only searches for example. This entire dance was to support `order:latest` which is not This also provides a proper allow list for non keyword searches which have always been supported unevenly. specifically @a should find by "a" even if we require 4 letters for a keyword search.
This commit is contained in:
@@ -20,7 +20,8 @@ class SearchController < ApplicationController
|
||||
# eg: ?q[foo]=bar
|
||||
raise Discourse::InvalidParameters.new(:q) if params[:q].present? && !@search_term.present?
|
||||
|
||||
if @search_term.present? && @search_term.length < SiteSetting.min_search_term_length
|
||||
if @search_term.present? && @search_term.length < SiteSetting.min_search_term_length &&
|
||||
!Search.valid_search_shortcut?(@search_term)
|
||||
raise Discourse::InvalidParameters.new(:q)
|
||||
end
|
||||
|
||||
|
||||
@@ -376,7 +376,12 @@ export default class FullPageSearchController extends Controller {
|
||||
|
||||
this.set("invalidSearch", false);
|
||||
const searchTerm = this.searchTerm;
|
||||
if (!isValidSearchTerm(searchTerm, this.siteSettings)) {
|
||||
// A non-zero sortOrder means user selected an order filter, which is valid even without a search term
|
||||
const hasValidSortOrder = this.sortOrder > 0;
|
||||
if (
|
||||
!hasValidSortOrder &&
|
||||
!isValidSearchTerm(searchTerm, this.siteSettings)
|
||||
) {
|
||||
this.set("invalidSearch", true);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -221,12 +221,24 @@ export function getSearchKey(args) {
|
||||
);
|
||||
}
|
||||
|
||||
// Patterns that indicate a valid search even without a traditional search term
|
||||
const SEARCH_SYNTAX_PATTERNS =
|
||||
/^(l|r|t)$|order:|category:|categories:|tags?:|before:|after:|status:|user:|group:|badge:|in:|with:|#|@/i;
|
||||
|
||||
export function isValidSearchTerm(searchTerm, siteSettings) {
|
||||
if (searchTerm) {
|
||||
return searchTerm.trim().length >= siteSettings.min_search_term_length;
|
||||
} else {
|
||||
if (!searchTerm) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const trimmed = searchTerm.trim();
|
||||
|
||||
// If the search contains filter syntax, it's valid regardless of length
|
||||
if (SEARCH_SYNTAX_PATTERNS.test(trimmed)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Otherwise, apply minimum length requirement
|
||||
return trimmed.length >= siteSettings.min_search_term_length;
|
||||
}
|
||||
|
||||
export function applySearchAutocomplete(inputElement, siteSettings, owner) {
|
||||
|
||||
+28
-5
@@ -27,6 +27,16 @@ class Search
|
||||
%w[topic category user private_messages tags all_topics exclude_topics]
|
||||
end
|
||||
|
||||
# Patterns that indicate a valid search even without meeting minimum term length
|
||||
# Includes shortcuts (l, r, t) and advanced filter syntax
|
||||
VALID_SEARCH_SHORTCUT_PATTERN =
|
||||
/\A[lrt]\z|order:|category:|categories:|tags?:|before:|after:|status:|user:|group:|badge:|in:|with:|#|@/i
|
||||
|
||||
def self.valid_search_shortcut?(term)
|
||||
return false if term.blank?
|
||||
VALID_SEARCH_SHORTCUT_PATTERN.match?(term)
|
||||
end
|
||||
|
||||
def self.ts_config(locale = SiteSetting.default_locale)
|
||||
# if adding a text search configuration, you should check PG beforehand:
|
||||
# SELECT cfgname FROM pg_ts_config;
|
||||
@@ -328,12 +338,12 @@ class Search
|
||||
@results.search_log_id = search_log_id unless status == :error
|
||||
end
|
||||
|
||||
unless @filters.present? || @opts[:search_for_id]
|
||||
min_length = min_search_term_length
|
||||
terms = (@term || "").split(/\s(?=(?:[^"]|"[^"]*")*$)/).reject { |t| t.length < min_length }
|
||||
is_topic_search = @search_context.present? && @search_context.is_a?(Topic)
|
||||
|
||||
if terms.blank?
|
||||
@term = ""
|
||||
if !@opts[:search_for_id] && !is_topic_search
|
||||
@term = filter_short_terms(@term)
|
||||
|
||||
if @term.blank? && @filters.blank? && @order.blank?
|
||||
@valid = false
|
||||
return
|
||||
end
|
||||
@@ -1628,6 +1638,19 @@ class Search
|
||||
@opts[:type_filter] != "exclude_topics"
|
||||
end
|
||||
|
||||
def filter_short_terms(term_string)
|
||||
return "" if term_string.blank?
|
||||
|
||||
min_length = min_search_term_length
|
||||
# Split on spaces but respect quoted phrases
|
||||
terms = term_string.split(/\s(?=(?:[^"]|"[^"]*")*$)/)
|
||||
|
||||
# Keep quoted phrases regardless of length, filter others by min_length
|
||||
valid_terms = terms.select { |t| t.start_with?('"') || t.length >= min_length }
|
||||
|
||||
valid_terms.join(" ")
|
||||
end
|
||||
|
||||
def min_search_term_length
|
||||
return @opts[:min_search_term_length] if @opts[:min_search_term_length]
|
||||
|
||||
|
||||
@@ -133,22 +133,17 @@ module DiscourseAi
|
||||
)
|
||||
|
||||
@last_num_results = results[:rows]&.length || 0
|
||||
@last_filter_query = results[:filter_query]
|
||||
results
|
||||
end
|
||||
|
||||
protected
|
||||
|
||||
def description_args
|
||||
# Use /filter when we fell back to TopicsFilter, otherwise use /search
|
||||
url =
|
||||
if @last_filter_query.present?
|
||||
"#{Discourse.base_path}/filter?q=#{CGI.escape(@last_filter_query)}"
|
||||
else
|
||||
"#{Discourse.base_path}/search?q=#{CGI.escape(@last_query || "")}"
|
||||
end
|
||||
|
||||
{ count: @last_num_results || 0, query: @last_query || "", url: url }
|
||||
{
|
||||
count: @last_num_results || 0,
|
||||
query: @last_query || "",
|
||||
url: "#{Discourse.base_path}/search?q=#{CGI.escape(@last_query || "")}",
|
||||
}
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
@@ -25,24 +25,6 @@ module DiscourseAi
|
||||
raise ArgumentError, "max_results must be a positive integer" if max_results <= 0
|
||||
max_results = MAX_RESULTS_LIMIT if max_results > MAX_RESULTS_LIMIT
|
||||
|
||||
if search_query.blank? &&
|
||||
has_any_filter?(category, user, order, tags, before, after, status)
|
||||
return(
|
||||
fallback_to_filter(
|
||||
category:,
|
||||
user:,
|
||||
order:,
|
||||
tags:,
|
||||
before:,
|
||||
after:,
|
||||
status:,
|
||||
max_results:,
|
||||
current_user:,
|
||||
result_style:,
|
||||
)
|
||||
)
|
||||
end
|
||||
|
||||
search_terms = []
|
||||
|
||||
search_terms << search_query.strip if search_query.present?
|
||||
@@ -146,120 +128,6 @@ module DiscourseAi
|
||||
end
|
||||
end
|
||||
|
||||
def self.order_to_filter_syntax(order)
|
||||
case order&.to_s
|
||||
when "latest", "latest_topic"
|
||||
"order:activity"
|
||||
when "oldest"
|
||||
"order:created-asc"
|
||||
when "views"
|
||||
"order:views"
|
||||
when "likes"
|
||||
"order:likes"
|
||||
end
|
||||
end
|
||||
|
||||
def self.has_any_filter?(category, user, order, tags, before, after, status)
|
||||
[category, user, order, tags, before, after, status].any?(&:present?)
|
||||
end
|
||||
|
||||
def self.fallback_to_filter(
|
||||
category:,
|
||||
user:,
|
||||
order:,
|
||||
tags:,
|
||||
before:,
|
||||
after:,
|
||||
status:,
|
||||
max_results:,
|
||||
current_user:,
|
||||
result_style:
|
||||
)
|
||||
guardian = Guardian.new(current_user)
|
||||
|
||||
query_parts = []
|
||||
query_parts << "category:#{category}" if category.present?
|
||||
if order.present? && order_to_filter_syntax(order)
|
||||
query_parts << order_to_filter_syntax(order)
|
||||
end
|
||||
query_parts << "tags:#{tags}" if tags.present?
|
||||
query_parts << "users:#{user}" if user.present?
|
||||
query_parts << "created-before:#{before}" if before.present?
|
||||
query_parts << "created-after:#{after}" if after.present?
|
||||
query_parts << "status:#{status}" if status.present?
|
||||
|
||||
return empty_results if query_parts.blank?
|
||||
|
||||
scope = TopicQuery.new(guardian.user).latest_results(skip_ordering: true)
|
||||
filter = TopicsFilter.new(guardian:, scope: scope)
|
||||
topics = filter.filter_from_query_string(query_parts.join(" "))
|
||||
|
||||
# may be uneeded with goldiloader
|
||||
topics = topics.includes(:category, :user)
|
||||
topics = topics.includes(:tags) if SiteSetting.tagging_enabled
|
||||
topics = topics.limit(max_results)
|
||||
|
||||
format_filter_results(
|
||||
topics,
|
||||
query_string: query_parts.join(" "),
|
||||
result_style:,
|
||||
category:,
|
||||
user:,
|
||||
order:,
|
||||
tags:,
|
||||
before:,
|
||||
after:,
|
||||
status:,
|
||||
max_results:,
|
||||
)
|
||||
end
|
||||
|
||||
def self.format_filter_results(
|
||||
topics,
|
||||
query_string:,
|
||||
result_style:,
|
||||
category:,
|
||||
user:,
|
||||
order:,
|
||||
tags:,
|
||||
before:,
|
||||
after:,
|
||||
status:,
|
||||
max_results:
|
||||
)
|
||||
search_args = {
|
||||
category:,
|
||||
user:,
|
||||
order:,
|
||||
tags:,
|
||||
before:,
|
||||
after:,
|
||||
status:,
|
||||
max_results:,
|
||||
}.compact
|
||||
|
||||
if topics.blank?
|
||||
{
|
||||
args: search_args,
|
||||
rows: [],
|
||||
instruction: "nothing was found, expand your search",
|
||||
filter_query: query_string,
|
||||
}
|
||||
else
|
||||
hidden_tags = DiscourseTagging.hidden_tag_names if SiteSetting.tagging_enabled
|
||||
result =
|
||||
format_results(topics, args: search_args, result_style: result_style) do |topic|
|
||||
format_row(topic: topic, hidden_tags:)
|
||||
end
|
||||
result[:filter_query] = query_string
|
||||
result
|
||||
end
|
||||
end
|
||||
|
||||
def self.empty_results
|
||||
{ args: {}, rows: [], instruction: "nothing was found, expand your search" }
|
||||
end
|
||||
|
||||
def self.format_row(topic:, post: nil, hidden_tags: nil)
|
||||
row = {
|
||||
title: topic.title,
|
||||
|
||||
@@ -103,14 +103,14 @@ RSpec.describe DiscourseAi::Personas::Tools::Search do
|
||||
expect(results[:rows]).to eq([])
|
||||
end
|
||||
|
||||
it "uses /filter URL for filter-only queries" do
|
||||
it "uses /search URL for filter-only queries" do
|
||||
Fabricate(:post, topic: topic_with_tags)
|
||||
|
||||
search = described_class.new({ order: "latest" }, bot_user: bot_user, llm: llm)
|
||||
search.invoke(&progress_blk)
|
||||
|
||||
description_args = search.send(:description_args)
|
||||
expect(description_args[:url]).to eq("/filter?q=order%3Aactivity")
|
||||
expect(description_args[:url]).to eq("/search?q=order%3Alatest")
|
||||
end
|
||||
|
||||
it "uses /search URL for queries with search terms" do
|
||||
|
||||
@@ -230,25 +230,31 @@ RSpec.describe DiscourseAi::Utils::Search do
|
||||
fab!(:post1) { Fabricate(:post, topic: topic1) }
|
||||
fab!(:post2) { Fabricate(:post, topic: topic2) }
|
||||
fab!(:post3) { Fabricate(:post, topic: topic3) }
|
||||
fab!(:post_with_tags) { Fabricate(:post, topic: topic_with_tags) }
|
||||
|
||||
it "returns topics with order:latest filter only (uses TopicsFilter fallback)" do
|
||||
before do
|
||||
# Ensure posts are indexed for search (fab! creates before SearchIndexer.enable)
|
||||
[post1, post2, post3, post_with_tags].each { |post| SearchIndexer.index(post, force: true) }
|
||||
end
|
||||
|
||||
it "returns posts with order:latest filter only" do
|
||||
results = described_class.perform_search(order: "latest", current_user: admin)
|
||||
|
||||
expect(results[:rows]).to be_present
|
||||
expect(results[:args][:order]).to eq("latest")
|
||||
|
||||
url_index = results[:column_names].index("url")
|
||||
topic_urls = results[:rows].map { |row| row[url_index] }
|
||||
post_urls = results[:rows].map { |row| row[url_index] }
|
||||
|
||||
expected_urls = [topic3.relative_url, topic2.relative_url, topic1.relative_url]
|
||||
expected_urls = [post3.url, post2.url, post1.url]
|
||||
|
||||
# keep only topics we expect (ignore any other fabricated topics)
|
||||
topic_urls &= expected_urls
|
||||
# keep only posts we expect (ignore any other fabricated posts)
|
||||
post_urls &= expected_urls
|
||||
|
||||
expect(topic_urls).to eq(expected_urls)
|
||||
expect(post_urls).to eq(expected_urls)
|
||||
end
|
||||
|
||||
it "returns topics filtered by category with order" do
|
||||
it "returns posts filtered by category with order" do
|
||||
results =
|
||||
described_class.perform_search(
|
||||
category: category.slug,
|
||||
@@ -259,52 +265,54 @@ RSpec.describe DiscourseAi::Utils::Search do
|
||||
expect(results[:rows]).to be_present
|
||||
|
||||
url_index = results[:column_names].index("url")
|
||||
topic_urls = results[:rows].map { |row| row[url_index] }
|
||||
post_urls = results[:rows].map { |row| row[url_index] }
|
||||
|
||||
expected_urls =
|
||||
Topic.order("views desc, bumped_at desc").where(category: category).map(&:relative_url)
|
||||
expect(topic_urls).to eq(expected_urls)
|
||||
# Search returns posts, ordered by topic views (topic_with_tags also in category, has 0 views)
|
||||
expected_urls = [post1.url, post2.url, post_with_tags.url]
|
||||
expect(post_urls).to eq(expected_urls)
|
||||
end
|
||||
|
||||
it "returns topics filtered by tags" do
|
||||
it "returns posts filtered by tags" do
|
||||
results = described_class.perform_search(tags: tag_funny.name, current_user: admin)
|
||||
|
||||
expect(results[:rows]).to be_present
|
||||
|
||||
url_index = results[:column_names].index("url")
|
||||
topic_urls = results[:rows].map { |row| row[url_index] }
|
||||
expect(topic_urls).to contain_exactly(topic_with_tags.relative_url, topic3.relative_url)
|
||||
post_urls = results[:rows].map { |row| row[url_index] }
|
||||
expect(post_urls).to contain_exactly(post_with_tags.url, post3.url)
|
||||
end
|
||||
|
||||
it "returns topics filtered by user" do
|
||||
it "returns posts filtered by user" do
|
||||
results = described_class.perform_search(user: post1.user.username, current_user: admin)
|
||||
|
||||
url_index = results[:column_names].index("url")
|
||||
topic_urls = results[:rows].map { |row| row[url_index] }
|
||||
post_urls = results[:rows].map { |row| row[url_index] }
|
||||
|
||||
expect(topic_urls).to contain_exactly(topic1.relative_url)
|
||||
expect(post_urls).to contain_exactly(post1.url)
|
||||
end
|
||||
|
||||
it "returns empty results when no filters are provided and no search query" do
|
||||
results = described_class.perform_search(current_user: admin)
|
||||
|
||||
# Search requires at least a term, filter, or order
|
||||
expect(results[:rows]).to eq([])
|
||||
end
|
||||
|
||||
it "respects category permissions" do
|
||||
private_topic = Fabricate(:topic, category: private_category)
|
||||
Fabricate(:post, topic: private_topic)
|
||||
private_post = Fabricate(:post, topic: private_topic)
|
||||
SearchIndexer.index(private_post, force: true)
|
||||
|
||||
results = described_class.perform_search(order: "latest", current_user: user)
|
||||
url_index = results[:column_names].index("url")
|
||||
topic_urls = results[:rows].map { |row| row[url_index] }.join
|
||||
expect(topic_urls).not_to include("/t/#{private_topic.slug}/#{private_topic.id}")
|
||||
post_urls = results[:rows].map { |row| row[url_index] }.join
|
||||
expect(post_urls).not_to include("/t/#{private_topic.slug}/#{private_topic.id}")
|
||||
|
||||
GroupUser.create!(group: group, user: user)
|
||||
results = described_class.perform_search(order: "latest", current_user: user)
|
||||
url_index = results[:column_names].index("url")
|
||||
topic_urls = results[:rows].map { |row| row[url_index] }.join
|
||||
expect(topic_urls).to include("/t/#{private_topic.slug}/#{private_topic.id}")
|
||||
post_urls = results[:rows].map { |row| row[url_index] }.join
|
||||
expect(post_urls).to include("/t/#{private_topic.slug}/#{private_topic.id}")
|
||||
end
|
||||
|
||||
it "returns correct result structure for filter-only queries with category" do
|
||||
@@ -339,7 +347,7 @@ RSpec.describe DiscourseAi::Utils::Search do
|
||||
results = described_class.perform_search(user: post1.user.username, current_user: admin)
|
||||
|
||||
url_index = results[:column_names].index("url")
|
||||
expect(results[:rows][0][url_index]).to eq("/subfolder/t/#{topic1.slug}/#{topic1.id}")
|
||||
expect(results[:rows][0][url_index]).to include("/subfolder/t/")
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
+67
-2
@@ -517,11 +517,43 @@ RSpec.describe Search do
|
||||
expect(search.term).to eq('"a b c d"')
|
||||
end
|
||||
|
||||
it "searches for short terms if one hits the length" do
|
||||
it "strips short terms but keeps valid ones" do
|
||||
search = Search.new("a b c okaylength", min_search_term_length: 5)
|
||||
search.execute
|
||||
expect(search.valid?).to eq(true)
|
||||
expect(search.term).to eq("a b c okaylength")
|
||||
expect(search.term).to eq("okaylength")
|
||||
end
|
||||
|
||||
describe "min_search_term_length with filters" do
|
||||
it "strips short terms even when filters are present" do
|
||||
search = Search.new("status:open ab", min_search_term_length: 3)
|
||||
search.execute
|
||||
expect(search.valid?).to eq(true)
|
||||
expect(search.term).to eq("")
|
||||
end
|
||||
|
||||
it "keeps valid terms when filters are present" do
|
||||
search = Search.new("status:open valid", min_search_term_length: 3)
|
||||
search.execute
|
||||
expect(search.valid?).to eq(true)
|
||||
expect(search.term).to eq("valid")
|
||||
end
|
||||
|
||||
it "strips short terms with order present" do
|
||||
search = Search.new("order:latest ab", min_search_term_length: 3)
|
||||
search.execute
|
||||
expect(search.valid?).to eq(true)
|
||||
expect(search.term).to eq("")
|
||||
end
|
||||
|
||||
it "allows short terms for in-topic search" do
|
||||
topic = Fabricate(:topic)
|
||||
Fabricate(:post, topic: topic, raw: "hello world")
|
||||
search = Search.new("a", min_search_term_length: 3, search_context: topic)
|
||||
search.execute
|
||||
expect(search.valid?).to eq(true)
|
||||
expect(search.term).to eq("a")
|
||||
end
|
||||
end
|
||||
|
||||
describe "query sanitization" do
|
||||
@@ -1472,6 +1504,39 @@ RSpec.describe Search do
|
||||
end
|
||||
end
|
||||
|
||||
context "with order-only searches" do
|
||||
it "returns results when searching with order and category filters" do
|
||||
result =
|
||||
Search.execute("order:latest category:#{topic.category.slug}", type_filter: "topic")
|
||||
|
||||
expect(result.posts).to be_present
|
||||
expect(result.posts.map(&:topic_id)).to include(topic.id)
|
||||
end
|
||||
|
||||
it "returns results when searching with only order filter" do
|
||||
post # ensure post is created
|
||||
|
||||
result = Search.execute("order:latest", type_filter: "topic")
|
||||
|
||||
expect(result.posts).to be_present
|
||||
end
|
||||
|
||||
it "returns results when using 'l' shortcut for order:latest" do
|
||||
post # ensure post is created
|
||||
|
||||
result = Search.execute("l", type_filter: "topic")
|
||||
|
||||
expect(result.posts).to be_present
|
||||
end
|
||||
|
||||
it "marks search as invalid when no term, filters, or order provided" do
|
||||
search = Search.new("", type_filter: "topic")
|
||||
search.execute
|
||||
|
||||
expect(search.valid?).to eq(false)
|
||||
end
|
||||
end
|
||||
|
||||
context "with security" do
|
||||
def result(current_user)
|
||||
Search.execute("hello", guardian: Guardian.new(current_user))
|
||||
|
||||
Reference in New Issue
Block a user