fix up a bit how searchable_by works in the real world (#7)

* fix up a bit how searchable_by works in the real world

* ruby lint

* make sure chewy is enabled

* Bring in a change from other pr so we can pass tests

* Try to get a cleaner query to work. This is going to fail some tests...

* clean up and lint

* more linting cause i goofed up that last one

* Try to add tests even though I don't know how to do that...

* wait...do i need to apply this?

* Try to make better tests

* colon on wrong side ya dingbat

* missed these last time too....

* Hopefully last round of linting...

* I think that should fix this problem

* I think all these tests were going to fail anyways...

* Let's move the lengths inside the paren cause otherwise what are we even doing...

* oops. Lost my .to's there

* these are arrays so we need the element of the array in order for us to actually get data from it I think

* fix some of the tests and add a new one

* Lint a ton more...

* Try to pass the filter tests and add one more final test

* try to add a bit of debugs so I can see what's up

* bluch. puts don't work...

* let's try a slightly more explict search term

* I think I should be passing all the tests now

* I just need the accounts, I don't need to assign it to anything

app/lib/search_query_transformer.rb

@@ -12,12 +12,34 @@ def initialize(clauses)
       @filter_clauses = grouped.fetch(:filter, [])
     end
 
-    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)
+    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
     end
 
     private

app/models/concerns/account_statuses_search.rb

@@ -4,6 +4,11 @@ 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

app/services/search_service.rb

@@ -39,25 +39,15 @@ def perform_accounts_search!
   end
 
   def perform_statuses_search!
-    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
-    []
+    StatusSearchService.new.call(
+      @query,
+      @account,
+      limit: @limit,
+      offset: @offset,
+      account_id: @options[:account_id],
+      min_id: @options[:min_id],
+      max_id: @options[:max_id]
+    )
   end
 
   def perform_hashtags_search!
@@ -114,8 +104,4 @@ 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

app/services/status_search_service.rb

@@ -0,0 +1,85 @@
+# 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

spec/lib/search_query_transformer_spec.rb

@@ -15,4 +15,111 @@
       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:@[email protected]' }
+
+      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:@[email protected]' }
+      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:@[email protected]' }
+      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