From e9eb9f315199497d47ee318156d8ddd373ccf381 Mon Sep 17 00:00:00 2001 From: jsgoldstein Date: Fri, 21 Jul 2023 04:23:41 -1000 Subject: [PATCH] Revert "fix up a bit how searchable_by works in the real world (#7)" This reverts commit 307ea707b9300ce19e92e6cdf8045cc64295a947. --- app/lib/search_query_transformer.rb | 34 +----- .../concerns/account_statuses_search.rb | 5 - app/services/search_service.rb | 32 ++++-- app/services/status_search_service.rb | 85 -------------- spec/lib/search_query_transformer_spec.rb | 107 ------------------ 5 files changed, 29 insertions(+), 234 deletions(-) delete mode 100644 app/services/status_search_service.rb diff --git a/app/lib/search_query_transformer.rb b/app/lib/search_query_transformer.rb index 17ae3d68d00d84..aef05e9d9d883c 100644 --- a/app/lib/search_query_transformer.rb +++ b/app/lib/search_query_transformer.rb @@ -12,34 +12,12 @@ def initialize(clauses) @filter_clauses = grouped.fetch(:filter, []) end - def apply(query) - query[:bool] ||= {} - - if should_clauses.present? - query[:bool][:should] ||= [] - - query[:bool][:should] += should_clauses.map { |clause| clause_to_query(clause) } - query[:bool][:minimum_should_match] = 1 - end - - if must_clauses.present? - query[:bool][:must] ||= [] - - query[:bool][:must] += must_clauses.map { |clause| clause_to_query(clause) } - end - - if must_not_clauses.present? - query[:bool][:must_not] ||= [] - - query[:bool][:must_not] += must_not_clauses.map { |clause| clause_to_query(clause) } - end - - if filter_clauses.present? - query[:bool][:filter] ||= [] - query[:bool][:filter] += filter_clauses.map { |clause| clause_to_filter(clause) } - end - - query + def apply(search) + should_clauses.each { |clause| search = search.query.should(clause_to_query(clause)) } + must_clauses.each { |clause| search = search.query.must(clause_to_query(clause)) } + must_not_clauses.each { |clause| search = search.query.must_not(clause_to_query(clause)) } + filter_clauses.each { |clause| search = search.filter(**clause_to_filter(clause)) } + search.query.minimum_should_match(1) end private diff --git a/app/models/concerns/account_statuses_search.rb b/app/models/concerns/account_statuses_search.rb index 0efcf1b5318075..99b4a7a3898450 100644 --- a/app/models/concerns/account_statuses_search.rb +++ b/app/models/concerns/account_statuses_search.rb @@ -4,11 +4,6 @@ module AccountStatusesSearch extend ActiveSupport::Concern def update_statuses_index! - return unless Chewy.enabled? - - # This might not scale if a user has a TON of statuses. - # If that is the case, perhaps for users with many statuses, we should: - # (1) get all their statuses and (2) submit requests to ES in batches. Chewy.strategy(:sidekiq) do StatusesIndex.import(query: Status.where(account_id: id)) end diff --git a/app/services/search_service.rb b/app/services/search_service.rb index 5fd485fbf10dc8..778ea85fb46fdf 100644 --- a/app/services/search_service.rb +++ b/app/services/search_service.rb @@ -39,15 +39,25 @@ def perform_accounts_search! end def perform_statuses_search! - StatusSearchService.new.call( - @query, - @account, - limit: @limit, - offset: @offset, - account_id: @options[:account_id], - min_id: @options[:min_id], - max_id: @options[:max_id] - ) + definition = parsed_query.apply(StatusesIndex.filter(term: { searchable_by: @account.id })) + + definition = definition.filter(term: { account_id: @options[:account_id] }) if @options[:account_id].present? + + if @options[:min_id].present? || @options[:max_id].present? + range = {} + range[:gt] = @options[:min_id].to_i if @options[:min_id].present? + range[:lt] = @options[:max_id].to_i if @options[:max_id].present? + definition = definition.filter(range: { id: range }) + end + + results = definition.limit(@limit).offset(@offset).objects.compact + account_ids = results.map(&:account_id) + account_domains = results.map(&:account_domain) + preloaded_relations = @account.relations_map(account_ids, account_domains) + + results.reject { |status| StatusFilter.new(status, @account, preloaded_relations).filtered? } + rescue Faraday::ConnectionFailed, Parslet::ParseFailed + [] end def perform_hashtags_search! @@ -104,4 +114,8 @@ def hashtag_search? def statuses_search? @options[:type].blank? || @options[:type] == 'statuses' end + + def parsed_query + SearchQueryTransformer.new.apply(SearchQueryParser.new.parse(@query)) + end end diff --git a/app/services/status_search_service.rb b/app/services/status_search_service.rb deleted file mode 100644 index 2a79581f216183..00000000000000 --- a/app/services/status_search_service.rb +++ /dev/null @@ -1,85 +0,0 @@ -# frozen_string_literal: true - -class StatusSearchService < BaseService - def call(query, account = nil, options = {}) - @query = query&.strip - @account = account - @options = options - @limit = options[:limit].to_i - @offset = options[:offset].to_i - - status_search_results - end - - private - - def status_search_results - definition = search_definition - - definition = definition.filter(term: { account_id: @options[:account_id] }) if @options[:account_id].present? - - if @options[:min_id].present? || @options[:max_id].present? - range = {} - range[:gt] = @options[:min_id].to_i if @options[:min_id].present? - range[:lt] = @options[:max_id].to_i if @options[:max_id].present? - definition = definition.filter(range: { id: range }) - end - - results = definition.limit(@limit).offset(@offset).objects.compact - account_ids = results.map(&:account_id) - account_domains = results.map(&:account_domain) - preloaded_relations = @account.relations_map(account_ids, account_domains) - - results.reject { |status| StatusFilter.new(status, @account, preloaded_relations).filtered? } - rescue Faraday::ConnectionFailed, Parslet::ParseFailed - [] - end - - def search_definition - non_publicly_searchable_clauses = non_publicly_searchable - publicly_searchable_clauses = publicly_searchable - - filter = { - bool: { - should: [ - non_publicly_searchable_clauses, - publicly_searchable_clauses, - ], - minimum_should_match: 1, - }, - } - - StatusesIndex.query(filter) - end - - def publicly_searchable - parsed_query.apply( - { - bool: { - must: [ - { term: { publicly_searchable: true } }, - ], - }, - } - ) - end - - def non_publicly_searchable - parsed_query.apply( - { - bool: { - must: [ - { term: { publicly_searchable: false } }, - ], - filter: [ - { term: { searchable_by: @account.id } }, - ], - }, - } - ) - end - - def parsed_query - SearchQueryTransformer.new.apply(SearchQueryParser.new.parse(@query)) - end -end diff --git a/spec/lib/search_query_transformer_spec.rb b/spec/lib/search_query_transformer_spec.rb index 3a71bbeea75f87..1095334695485d 100644 --- a/spec/lib/search_query_transformer_spec.rb +++ b/spec/lib/search_query_transformer_spec.rb @@ -15,111 +15,4 @@ expect(transformer.filter_clauses.first).to be_nil end end - - describe '#apply' do - subject(:applied_query) do - described_class.new.apply( - SearchQueryParser.new.parse(search_term) - ).apply(query) - end - - let(:query) { { bool: {} } } - let(:search_term) { '' } - - context 'when query is just a bool' do - it 'returns a hash of bool' do - expect(applied_query).to eq(query) - end - end - - context 'when should_clauses are present' do - let(:search_term) { 'test' } - - it 'adds should clauses to the query' do - expect(applied_query[:bool][:should].length).to eq(1) - expect(applied_query[:bool][:should][0][:multi_match][:query]).to eq(search_term) - end - - it 'sets minimum_should_match to 1' do - expect(applied_query[:bool][:minimum_should_match]).to eq(1) - end - end - - context 'when must_clauses are present' do - let(:search_term) { '+test' } - - it 'adds must clauses to the query' do - expect(applied_query[:bool][:must].length).to eq(1) - expect(applied_query[:bool][:must][0][:multi_match][:query]).to eq(search_term[1..]) - end - end - - context 'when must_not_clauses are present' do - let(:search_term) { '-test' } - - it 'adds must_not clauses to the query' do - expect(applied_query[:bool][:must_not].length).to eq(1) - expect(applied_query[:bool][:must_not][0][:multi_match][:query]).to eq(search_term[1..]) - end - end - - context 'when filter_clauses are present' do - let(:search_term) { 'from:@test_account@example.com' } - - it 'adds filter clauses to the query' do - account = Fabricate(:account, username: 'test_account', domain: 'example.com') - - expect(applied_query[:bool][:filter].length).to eq(1) - expect(applied_query[:bool][:filter][0][:term][:account_id]).to eq(account.id) - puts "Final count of Account records: #{Account.count}" - end - end - - context 'when all clauses are present' do - let(:should_term) { 'should' } - let(:must_term) { '+must' } - let(:must_not_term) { '-nope' } - let(:filter_term) { 'from:@test_account@example.com' } - let(:search_term) { "#{should_term} #{must_term} #{must_not_term} #{filter_term}" } - - it 'adds all clauses to the query' do - account = Fabricate(:account, username: 'test_account', domain: 'example.com') - - expect(applied_query[:bool][:should][0][:multi_match][:query]).to eq(should_term) - expect(applied_query[:bool][:must][0][:multi_match][:query]).to eq(must_term[1..]) - expect(applied_query[:bool][:must_not][0][:multi_match][:query]).to eq(must_not_term[1..]) - expect(applied_query[:bool][:filter][0][:term][:account_id]).to eq(account.id) - end - - it 'sets minimum_should_match to 1' do - Fabricate(:account, username: 'test_account', domain: 'example.com') - - expect(applied_query[:bool][:minimum_should_match]).to eq(1) - end - end - - context 'when all clauses are present and query has all clauses too' do - let(:should_term) { 'should' } - let(:must_term) { '+must' } - let(:must_not_term) { '-nope' } - let(:filter_term) { 'from:@test_account@example.com' } - let(:search_term) { "#{should_term} #{must_term} #{must_not_term} #{filter_term}" } - let(:query) { { bool: { should: [{ term: { exists: true } }], must: [{ term: { exists: true } }], must_not: [{ term: { exists: true } }], filter: [{ term: { exists: true } }] } } } - - it 'adds all clauses to the arrays in the query' do - Fabricate(:account, username: 'test_account', domain: 'example.com') - - expect(applied_query[:bool][:should].length).to eq(2) - expect(applied_query[:bool][:must].length).to eq(2) - expect(applied_query[:bool][:must_not].length).to eq(2) - expect(applied_query[:bool][:filter].length).to eq(2) - end - - it 'sets minimum_should_match to 1' do - Fabricate(:account, username: 'test_account', domain: 'example.com') - - expect(applied_query[:bool][:minimum_should_match]).to eq(1) - end - end - end end