Merge pull request #152 from alphagov/filter-more

Simplify filter expression generation

app/services/discovery_engine/query/filter_expression_helpers.rb

@@ -0,0 +1,52 @@
+module DiscoveryEngine::Query
+  module FilterExpressionHelpers
+    # Creates a filter expression for documents where string_or_array_field contains any of the
+    # values in string_value_or_values
+    def any_string(string_or_array_field, string_value_or_values)
+      Array(string_value_or_values)
+        .map { escape_and_quote(_1) }
+        .join(",")
+        .then { "#{string_or_array_field}: ANY(#{_1})" }
+    end
+
+    # Creates a filter expression for documents where array_field contains all of the values in string_value_or_values
+    def all_string(array_field, string_value_or_values)
+      Array(string_value_or_values)
+        .map { any_string(array_field, _1) }
+        .then { conjunction(_1) }
+    end
+
+    # Creates a filter expression for documents where string_or_array_field does not contain any of the values in
+    # string_value_or_values
+    def not_string(string_or_array_field, string_value_or_values)
+      any_string(string_or_array_field, string_value_or_values)
+        .then { negate(_1) }
+    end
+
+    # Creates a filter expression from several expressions where all must be true
+    def conjunction(expression_or_expressions)
+      expressions = Array(expression_or_expressions).compact_blank
+      return expressions.first if expressions.one?
+
+      Array(expressions)
+        .map { parenthesize(_1) }
+        .join(" AND ")
+        .presence
+    end
+
+  private
+
+    def negate(expression)
+      "NOT #{expression}"
+    end
+
+    def parenthesize(expression)
+      "(#{expression})"
+    end
+
+    def escape_and_quote(string_value)
+      escaped_string = string_value.gsub(/(["\\])/, '\\\\\1')
+      "\"#{escaped_string}\""
+    end
+  end
+end

app/services/discovery_engine/query/filters.rb

@@ -2,21 +2,20 @@ module DiscoveryEngine::Query
   class Filters
     FILTERABLE_FIELDS = %i[content_purpose_supergroup link part_of_taxonomy_tree].freeze
 
+    include FilterExpressionHelpers
+
     def initialize(query_params)
       @query_params = query_params
     end
 
     def filter_expression
       expressions = [
-        *query_params_of_type(:reject).map { FilterExpressions::AnyStringFilterExpression.new(_1, _2).negated_expression },
-        *query_params_of_type(:filter).map { FilterExpressions::AnyStringFilterExpression.new(_1, _2).expression },
-        *query_params_of_type(:filter_all).map { FilterExpressions::AllStringFilterExpression.new(_1, _2).expression },
+        *query_params_of_type(:reject).map { not_string(_1, _2) },
+        *query_params_of_type(:filter).map { any_string(_1, _2) },
+        *query_params_of_type(:filter_all).map { all_string(_1, _2) },
       ].compact
 
-      expressions
-        .map { "(#{_1})" }
-        .join(" AND ")
-        .presence
+      conjunction(expressions)
     end
 
   private

spec/services/discovery_engine/query/filters_spec.rb

@@ -18,13 +18,13 @@
       context "with a single parameter" do
         let(:query_params) { { q: "garden centres", reject_link: "/foo" } }
 
-        it { is_expected.to eq('(NOT link: ANY("/foo"))') }
+        it { is_expected.to eq('NOT link: ANY("/foo")') }
       end
 
       context "with several parameters" do
         let(:query_params) { { q: "garden centres", reject_link: ["/foo", "/bar"] } }
 
-        it { is_expected.to eq('(NOT link: ANY("/foo","/bar"))') }
+        it { is_expected.to eq('NOT link: ANY("/foo","/bar")') }
       end
     end
 
@@ -40,15 +40,15 @@
           { q: "garden centres", filter_content_purpose_supergroup: "services" }
         end
 
-        it { is_expected.to eq('(content_purpose_supergroup: ANY("services"))') }
+        it { is_expected.to eq('content_purpose_supergroup: ANY("services")') }
       end
 
       context "with several parameters" do
         let(:query_params) do
           { q: "garden centres", filter_content_purpose_supergroup: %w[services guidance] }
         end
 
-        it { is_expected.to eq('(content_purpose_supergroup: ANY("services","guidance"))') }
+        it { is_expected.to eq('content_purpose_supergroup: ANY("services","guidance")') }
       end
     end
 
@@ -62,15 +62,15 @@
       context "with a single parameter" do
         let(:query_params) { { q: "garden centres", filter_all_part_of_taxonomy_tree: "cafe-1234" } }
 
-        it { is_expected.to eq('(part_of_taxonomy_tree: ANY("cafe-1234"))') }
+        it { is_expected.to eq('part_of_taxonomy_tree: ANY("cafe-1234")') }
       end
 
       context "with several parameters" do
         let(:query_params) do
           { q: "garden centres", filter_all_part_of_taxonomy_tree: %w[cafe-1234 face-5678] }
         end
 
-        it { is_expected.to eq('(part_of_taxonomy_tree: ANY("cafe-1234") AND part_of_taxonomy_tree: ANY("face-5678"))') }
+        it { is_expected.to eq('(part_of_taxonomy_tree: ANY("cafe-1234")) AND (part_of_taxonomy_tree: ANY("face-5678"))') }
       end
     end
 
@@ -84,13 +84,13 @@
         }
       end
 
-      it { is_expected.to eq('(NOT link: ANY("/foo")) AND (content_purpose_supergroup: ANY("services")) AND (part_of_taxonomy_tree: ANY("cafe-1234") AND part_of_taxonomy_tree: ANY("face-5678"))') }
+      it { is_expected.to eq('(NOT link: ANY("/foo")) AND (content_purpose_supergroup: ANY("services")) AND ((part_of_taxonomy_tree: ANY("cafe-1234")) AND (part_of_taxonomy_tree: ANY("face-5678")))') }
     end
 
     context "with filters containing escapable characters" do
       let(:query_params) { { q: "garden centres", filter_content_purpose_supergroup: "foo\"\\bar" } }
 
-      it { is_expected.to eq('(content_purpose_supergroup: ANY("foo\\"\\\\bar"))') }
+      it { is_expected.to eq('content_purpose_supergroup: ANY("foo\\"\\\\bar")') }
     end
   end
 end