Merge pull request #168 from alphagov/filter-warn

Improve handling of invalid query filters

app/services/discovery_engine/query/filter_expression_helpers.rb

@@ -29,7 +29,10 @@ def filter_not_string(string_or_array_field, string_value_or_values)
     # timestamp_value
     def filter_timestamp(timestamp_field, timestamp_value)
       match = timestamp_value.match(TIMESTAMP_VALUE_REGEX)
-      return nil unless match && (match[:from] || match[:to])
+      unless match && (match[:from] || match[:to])
+        Rails.logger.warn("#{self.class.name}: Invalid timestamp value: '#{timestamp_value}'")
+        return nil
+      end
 
       from = match[:from] ? Date.parse(match[:from]).beginning_of_day.to_i : "*"
       to = match[:to] ? Date.parse(match[:to]).end_of_day.to_i : "*"

app/services/discovery_engine/query/filters.rb

@@ -30,7 +30,18 @@ def parse_param(key, value)
       when *FILTERABLE_STRING_FIELDS
         string_filter_expression(filter_type, filter_field, value)
       when *FILTERABLE_TIMESTAMP_FIELDS
+        if filter_type != "filter"
+          Rails.logger.warn(
+            "#{self.class.name}: Cannot filter on timestamp field '#{filter_field}' " \
+            "with filter type '#{filter_type}'",
+          )
+          return nil
+        end
+
         filter_timestamp(filter_field, value)
+      else
+        Rails.logger.info("#{self.class.name}: Ignoring unknown filter field: '#{filter_field}'")
+        nil
       end
     end
 

spec/services/discovery_engine/query/filters_spec.rb

@@ -26,6 +26,12 @@
 
         it { is_expected.to eq('NOT link: ANY("/foo","/bar")') }
       end
+
+      context "with an unknown field" do
+        let(:query_params) { { q: "garden centres", reject_foo: "bar" } }
+
+        it { is_expected.to be_nil }
+      end
     end
 
     context "with an 'any' string filter" do
@@ -50,6 +56,12 @@
 
         it { is_expected.to eq('content_purpose_supergroup: ANY("services","guidance")') }
       end
+
+      context "with an unknown field" do
+        let(:query_params) { { q: "garden centres", filter_foo: "bar" } }
+
+        it { is_expected.to be_nil }
+      end
     end
 
     context "with an 'all' string filter" do
@@ -72,6 +84,12 @@
 
         it { is_expected.to eq('(part_of_taxonomy_tree: ANY("cafe-1234")) AND (part_of_taxonomy_tree: ANY("face-5678"))') }
       end
+
+      context "with an unknown field" do
+        let(:query_params) { { q: "garden centres", filter_all_foo: "bar" } }
+
+        it { is_expected.to be_nil }
+      end
     end
 
     context "with a timestamp filter" do
@@ -110,6 +128,18 @@
 
         it { is_expected.to be_nil }
       end
+
+      context "with an invalid filter_all type" do
+        let(:query_params) { { q: "garden centres", filter_all_public_timestamp: "from:1989-12-13" } }
+
+        it { is_expected.to be_nil }
+      end
+
+      context "with an invalid reject type" do
+        let(:query_params) { { q: "garden centres", reject_public_timestamp: "from:1989-12-13" } }
+
+        it { is_expected.to be_nil }
+      end
     end
 
     context "with several filters specified" do