Improve handling of invalid query filters

This will help us figure out during traffic replay and initial
operations whether there are any required filters we are still missing,
or arcane behaviours of the previous v1 API that we still need to
replicate.

- Add logging for filtering attempts on non-allowlisted fields
- Add logging for invalid timestamp values
- Add further tests to make sure our logging doesn't break return values
  or otherwise behave weirdly

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