From aabe64ae059ec093c639169f5bc59630306f6041 Mon Sep 17 00:00:00 2001
From: Christian Sutter <git@doublesignal.com>
Date: Fri, 5 Jan 2024 11:57:08 +0000
Subject: [PATCH] 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
---
 .../query/filter_expression_helpers.rb        |  5 +++-
 .../discovery_engine/query/filters.rb         | 11 +++++++
 .../discovery_engine/query/filters_spec.rb    | 30 +++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/app/services/discovery_engine/query/filter_expression_helpers.rb b/app/services/discovery_engine/query/filter_expression_helpers.rb
index b06d6e0..127a30d 100644
--- a/app/services/discovery_engine/query/filter_expression_helpers.rb
+++ b/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 : "*"
diff --git a/app/services/discovery_engine/query/filters.rb b/app/services/discovery_engine/query/filters.rb
index f26f0f3..9838844 100644
--- a/app/services/discovery_engine/query/filters.rb
+++ b/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
 
diff --git a/spec/services/discovery_engine/query/filters_spec.rb b/spec/services/discovery_engine/query/filters_spec.rb
index 57bf45c..9e0bdcc 100644
--- a/spec/services/discovery_engine/query/filters_spec.rb
+++ b/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