Fix/non matching pagination (#9202)

* fix duplicates returned for wp list on multiple matching attachments/comments

LEFT JOIN leads to potentially having multiple records returned if a search string is present in more than one attachment/comment. This is not a problem if we fetch work packages directly out of the query results but becomes problematic if only the id is fetched and then used to get the work packages by a subquery.

* attachment filters no longer differentiate on tsv_where concatenation

As EXISTS and NOT EXISTS is now used, the full string is always to be searched for so `and` concatenation is now always employed.

* adapt spec

Work packages that have no attachment at all are now also considered to match the filter if a "doesn`t contain" operator is used

* less strict rspec rubocops for feature specs

* cleanup code

.rubocop.yml

@@ -95,6 +95,23 @@ Naming/PredicateName:
   ForbiddenPrefixes:
     - is_
 
+# For feature specs, we tend to have longer specs that cover a larger part of the functionality.
+# This is done for multiple reasons:
+# * performance, as setting up integration tests is costly
+# * following a scenario that is closer to how a user interacts
+RSpec/ExampleLength:
+  Enabled: true
+  Exclude:
+    - 'spec/features/**/*.rb'
+    - 'modules/*/spec/features/**/*.rb'
+
+# See RSpec/ExampleLength for why feature specs are excluded
+RSpec/MultipleExpectations
+  Enabled: true
+  Exclude:
+    - 'spec/features/**/*.rb'
+    - 'modules/*/spec/features/**/*.rb'
+
 
 Style/Alias:
   Enabled: false

app/models/queries/work_packages/filter/attachment_base_filter.rb

@@ -29,19 +29,8 @@
 #++
 
 class Queries::WorkPackages::Filter::AttachmentBaseFilter < Queries::WorkPackages::Filter::WorkPackageFilter
-  include Queries::WorkPackages::Filter::FilterOnTsvMixin
   include Queries::WorkPackages::Filter::TextFilterOnJoinMixin
 
-  attr_reader :join_table_suffix
-
-  def initialize(name, options = {})
-    super name, options
-
-    # Generate a uniq suffix to add to the join table
-    # because attachment filters may be used multiple times
-    @join_table_suffix = SecureRandom.hex(4)
-  end
-
   def type
     :text
   end
@@ -50,33 +39,25 @@ class Queries::WorkPackages::Filter::AttachmentBaseFilter < Queries::WorkPackage
     EnterpriseToken.allows_to?(:attachment_filters) && OpenProject::Database.allows_tsv?
   end
 
-  def where
-    Queries::Operators::All.sql_for_field(values, join_table_alias, 'id')
-  end
-
   protected
 
-  def join_table_alias
-    "#{self.class.key}_#{join_table}_#{join_table_suffix}"
-  end
-
-  def join_table
-    Attachment.table_name
-  end
-
-  def join_condition
+  def where_condition
     <<-SQL
-      #{join_table_alias}.container_id = #{WorkPackage.table_name}.id
-      AND #{join_table_alias}.container_type = '#{WorkPackage.name}'
+      SELECT 1 FROM #{attachment_table}
+      WHERE #{attachment_table}.container_id = #{WorkPackage.table_name}.id
+      AND #{attachment_table}.container_type = '#{WorkPackage.name}'
       AND #{tsv_condition}
     SQL
   end
 
+  def attachment_table
+    Attachment.table_name
+  end
+
   def tsv_condition
-    OpenProject::FullTextSearch.tsv_where(join_table_alias,
+    OpenProject::FullTextSearch.tsv_where(attachment_table,
                                           search_column,
                                           values.first,
-                                          concatenation: concatenation,
                                           normalization: normalization_type)
   end
 

app/models/queries/work_packages/filter/category_filter.rb

@@ -35,8 +35,7 @@ class Queries::WorkPackages::Filter::CategoryFilter <
   end
 
   def available?
-    project &&
-      project.categories.exists?
+    project&.categories&.exists?
   end
 
   def type

app/models/queries/work_packages/filter/comment_filter.rb

@@ -35,21 +35,22 @@ class Queries::WorkPackages::Filter::CommentFilter < Queries::WorkPackages::Filt
     :text
   end
 
-  def join_condition
+  private
+
+  def where_condition
     <<-SQL
-     #{join_table_alias}.journable_id = #{WorkPackage.table_name}.id
-	   AND #{join_table_alias}.journable_type = '#{WorkPackage.name}'
+     SELECT 1 FROM #{journal_table}
+     WHERE #{journal_table}.journable_id = #{WorkPackage.table_name}.id
+	   AND #{journal_table}.journable_type = '#{WorkPackage.name}'
      AND #{notes_condition}
     SQL
   end
 
-  private
-
-  def join_table
-    Journal.table_name
+  def notes_condition
+    Queries::Operators::Contains.sql_for_field(values, journal_table, 'notes')
   end
 
-  def notes_condition
-    Queries::Operators::Contains.sql_for_field(values, join_table_alias, 'notes')
+  def journal_table
+    Journal.table_name
   end
 end

app/models/queries/work_packages/filter/filter_on_tsv_mixin.rb

@@ -1,40 +0,0 @@
-#-- encoding: UTF-8
-
-#-- copyright
-# OpenProject is an open source project management software.
-# Copyright (C) 2012-2021 the OpenProject GmbH
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License version 3.
-#
-# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
-# Copyright (C) 2006-2013 Jean-Philippe Lang
-# Copyright (C) 2010-2013 the ChiliProject Team
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License
-# as published by the Free Software Foundation; either version 2
-# of the License, or (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-#
-# See docs/COPYRIGHT.rdoc for more details.
-#++
-
-module Queries::WorkPackages::Filter::FilterOnTsvMixin
-  def concatenation
-    case operator
-    when '~'
-      :and
-    when '!~'
-      :and_not
-    end
-  end
-end

app/models/queries/work_packages/filter/or_filter_for_wp_mixin.rb

@@ -45,14 +45,6 @@ module Queries::WorkPackages::Filter::OrFilterForWpMixin
     @filters.keep_if(&:validate)
   end
 
-  def joins
-    filters.map(&:joins).flatten.compact
-  end
-
-  def includes
-    filters.map(&:includes).flatten.uniq.reject(&:blank?)
-  end
-
   def where
     filters.map(&:where).join(' OR ')
   end

app/models/queries/work_packages/filter/search_filter.rb

@@ -31,7 +31,6 @@
 class Queries::WorkPackages::Filter::SearchFilter <
   Queries::WorkPackages::Filter::WorkPackageFilter
   include Queries::WorkPackages::Filter::OrFilterForWpMixin
-  include Queries::WorkPackages::Filter::FilterOnTsvMixin
 
   CONTAINS_OPERATOR = '~'.freeze
 
@@ -66,14 +65,6 @@ class Queries::WorkPackages::Filter::SearchFilter <
     )
   ].freeze
 
-  def self.key
-    :search
-  end
-
-  def name
-    :search
-  end
-
   def type
     :search
   end

app/models/queries/work_packages/filter/text_filter_on_join_mixin.rb

@@ -32,32 +32,11 @@ module Queries::WorkPackages::Filter::TextFilterOnJoinMixin
   def where
     case operator
     when '~'
-      Queries::Operators::All.sql_for_field(values, join_table_alias, 'id')
+      "EXISTS (#{where_condition})"
     when '!~'
-      Queries::Operators::None.sql_for_field(values, join_table_alias, 'id')
+      "NOT EXISTS (#{where_condition})"
     else
       raise 'Unsupported operator'
     end
   end
-
-  def joins
-    <<-SQL
-     LEFT OUTER JOIN #{join_table} #{join_table_alias}
-     ON #{join_condition}
-    SQL
-  end
-
-  private
-
-  def join_table
-    raise NotImplementedError
-  end
-
-  def join_condition
-    raise NotImplementedError
-  end
-
-  def join_table_alias
-    "#{self.class.key}_#{join_table}"
-  end
 end

lib/open_project/full_text_search.rb

@@ -33,10 +33,10 @@ module OpenProject
   module FullTextSearch
     DISALLOWED_CHARACTERS = /['?\\:()&|!*<>]/
 
-    def self.tsv_where(table_name, column_name, value, options = { concatenation: :and, normalization: :text })
+    def self.tsv_where(table_name, column_name, value, concatenation: :and, normalization: :text)
       if OpenProject::Database.allows_tsv?
-        column = '"' + table_name.to_s + '"."' + column_name.to_s + '_tsv"'
-        query = tokenize(value, options[:concatenation], options[:normalization])
+        column = "\"#{table_name.to_s}\".\"#{column_name.to_s}_tsv\""
+        query = tokenize(value, concatenation, normalization)
         language = OpenProject::Configuration.main_content_language
 
         ActiveRecord::Base.send(
@@ -56,7 +56,7 @@ module OpenProject
         terms.join ' & '
       when :and_not
         # all terms must not hit.
-        '! ' + terms.join(' & ! ')
+        "! #{terms.join(' & ! ')}"
       end
     end
 

lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb

@@ -99,7 +99,6 @@ module Redmine
                 OpenProject::FullTextSearch.tsv_where(tsv_column[:table_name],
                                                       tsv_column[:column_name],
                                                       tokens.join(' '),
-                                                      concatenation: :and,
                                                       normalization: tsv_column[:normalization_type])
               end
             end

modules/backlogs/lib/open_project/backlogs/work_package_filter.rb

@@ -51,10 +51,6 @@ module OpenProject::Backlogs
       sql_for_field(values)
     end
 
-    def order
-      20
-    end
-
     def type
       :list
     end

modules/budgets/app/models/queries/work_packages/filter/budget_filter.rb

@@ -41,10 +41,6 @@ module Queries::WorkPackages::Filter
       :budget_id
     end
 
-    def order
-      14
-    end
-
     def type
       :list_optional
     end

spec/features/work_packages/table/queries/filter_spec.rb

@@ -407,8 +407,8 @@ describe 'filter work packages', js: true do
                               'attachmentContent')
 
         loading_indicator_saveguard
-        wp_table.expect_work_package_listed wp_with_attachment_b
-        wp_table.ensure_work_package_not_listed! wp_without_attachment, wp_with_attachment_a
+        wp_table.expect_work_package_listed wp_with_attachment_b, wp_without_attachment
+        wp_table.ensure_work_package_not_listed! wp_with_attachment_a
 
         filters.remove_filter 'attachmentContent'
 

spec/models/queries/work_packages/filter/attachment_content_filter_spec.rb

@@ -88,6 +88,13 @@ describe Queries::WorkPackages::Filter::AttachmentContentFilter, type: :model do
         end
       end
 
+      describe '#available_operators' do
+        it 'supports ~ and !~' do
+          expect(instance.available_operators)
+            .to eql [Queries::Operators::Contains, Queries::Operators::NotContains]
+        end
+      end
+
       it_behaves_like 'non ar filter'
     end
   end

spec/models/queries/work_packages/filter/attachment_file_name_filter_spec.rb

@@ -62,6 +62,13 @@ describe Queries::WorkPackages::Filter::AttachmentFileNameFilter, type: :model d
         end
       end
 
+      describe '#available_operators' do
+        it 'supports ~ and !~' do
+          expect(instance.available_operators)
+            .to eql [Queries::Operators::Contains, Queries::Operators::NotContains]
+        end
+      end
+
       it_behaves_like 'non ar filter'
     end
   end

spec/models/queries/work_packages/filter/comment_filter_spec.rb

@@ -56,6 +56,13 @@ describe Queries::WorkPackages::Filter::CommentFilter, type: :model do
       end
     end
 
+    describe '#available_operators' do
+      it 'supports ~ and !~' do
+        expect(instance.available_operators)
+          .to eql [Queries::Operators::Contains, Queries::Operators::NotContains]
+      end
+    end
+
     it_behaves_like 'non ar filter'
   end
 end

spec/models/queries/work_packages/filter/principal_loader_spec.rb

@@ -139,10 +139,6 @@ describe Queries::WorkPackages::Filter::PrincipalLoader, type: :model do
         let(:matching_principals) { [] }
 
         it 'is empty' do
-          allow(project)
-            .to receive(:principals)
-            .and_return([])
-
           expect(instance.principal_values).to be_empty
         end
       end

spec/models/queries/work_packages/filter/search_filter_spec.rb

@@ -128,6 +128,33 @@ describe Queries::WorkPackages::Filter::SearchFilter, type: :model do
               .to match_array [work_package]
           end
         end
+
+        context 'with two attachments' do
+          let(:text) { 'lorem ipsum' }
+          let(:filename) { 'plaintext-file.txt' }
+          let(:attachment) { FactoryBot.create(:attachment, container: work_package, filename: filename) }
+          let(:attachment2) { FactoryBot.create(:attachment, container: work_package, filename: filename) }
+
+          before do
+            allow_any_instance_of(Plaintext::Resolver).to receive(:text).and_return(text)
+            allow(attachment).to receive(:readable?).and_return(true)
+            allow(attachment2).to receive(:readable?).and_return(true)
+            work_package.reload
+          end
+
+          context 'with the search string in both attachments' do
+            let(:text) { 'plaintext lorem ipsum' }
+
+            it "only finds work package once" do
+              perform_enqueued_jobs
+
+              instance.values = ['plaintext']
+
+              expect(WorkPackage.joins(instance.joins).where(instance.where).pluck(:id))
+                .to match_array [work_package.id]
+            end
+          end
+        end
       end
 
       it_behaves_like 'basic query filter' do
@@ -157,6 +184,13 @@ describe Queries::WorkPackages::Filter::SearchFilter, type: :model do
           end
         end
 
+        describe '#available_operators' do
+          it 'supports **' do
+            expect(instance.available_operators)
+              .to eql [Queries::Operators::Everywhere]
+          end
+        end
+
         it_behaves_like 'non ar filter'
       end
     end