Refactor DiscoveryEngine::Search

- Pass entirety of query parameters to search (in anticipation of
  implementing more querying functionality)
- Move to a more OO-y rather than "callable" structure for the class and
  move parameters into object attributes
- Add general `permit` of params to Brakeman ignore file

Author: csutter

app/controllers/searches_controller.rb

@@ -1,17 +1,11 @@
 class SearchesController < ApplicationController
   def show
-    result_set = DiscoveryEngine::Search.new.call(
-      query_params[:q],
-      start: query_params[:start]&.to_i,
-      count: query_params[:count]&.to_i,
-    )
-
-    render json: result_set
+    render json: DiscoveryEngine::Search.new(query_params).result_set
   end
 
 private
 
   def query_params
-    params.permit(:q, :start, :count)
+    params.permit!
   end
 end

app/services/discovery_engine/search.rb

@@ -1,48 +1,60 @@
 module DiscoveryEngine
   class Search
-    DEFAULT_COUNT = 10
-    DEFAULT_START = 0
+    DEFAULT_PAGE_SIZE = 10
+    DEFAULT_OFFSET = 0
 
     include BestBetsBoost
     include NewsRecencyBoost
 
-    def initialize(client: ::Google::Cloud::DiscoveryEngine.search_service(version: :v1))
+    def initialize(
+      query_params,
+      client: ::Google::Cloud::DiscoveryEngine.search_service(version: :v1)
+    )
+      @query_params = query_params
       @client = client
     end
 
-    def call(query_string, start: nil, count: nil)
-      count ||= DEFAULT_COUNT
-      start ||= DEFAULT_START
-      query_string ||= ""
-
+    def result_set
       response = client.search(
-        query: query_string,
+        query:,
         serving_config:,
-        page_size: count,
-        offset: start,
-        boost_spec: boost_spec(query_string),
+        page_size:,
+        offset:,
+        boost_spec:,
       ).response
 
       ResultSet.new(
         results: response.results.map { Result.from_stored_document(_1.document.struct_data.to_h) },
         total: response.total_size,
-        start:,
+        start: offset,
       )
     end
 
   private
 
-    attr_reader :client
+    attr_reader :query_params, :client
+
+    def query
+      query_params[:q].presence || ""
+    end
+
+    def page_size
+      query_params[:count].presence&.to_i || DEFAULT_PAGE_SIZE
+    end
+
+    def offset
+      query_params[:start].presence&.to_i || DEFAULT_OFFSET
+    end
 
     def serving_config
       Rails.configuration.discovery_engine_serving_config
     end
 
-    def boost_spec(query_string)
+    def boost_spec
       {
         condition_boost_specs: [
           *news_recency_boost_specs,
-          *best_bets_boost_specs(query_string),
+          *best_bets_boost_specs(query),
         ],
       }
     end

config/brakeman.ignore

@@ -0,0 +1,29 @@
+{
+  "ignored_warnings": [
+    {
+      "warning_type": "Mass Assignment",
+      "warning_code": 70,
+      "fingerprint": "4c13d7998d2abdaad68d35c884f9a4527c1c3085beda87c99a93ada0eaef496e",
+      "check_name": "MassAssignment",
+      "message": "Specify exact keys allowed for mass assignment instead of using `permit!` which allows any keys",
+      "file": "app/controllers/searches_controller.rb",
+      "line": 9,
+      "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
+      "code": "params.permit!",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "SearchesController",
+        "method": "query_params"
+      },
+      "user_input": null,
+      "confidence": "Medium",
+      "cwe_id": [
+        915
+      ],
+      "note": "Mass assignment not a risk as not using Active Record and parsing these parameters elsewhere"
+    }
+  ],
+  "updated": "2023-12-06 16:07:36 +0000",
+  "brakeman_version": "6.1.0"
+}

spec/requests/search_request_spec.rb

@@ -1,5 +1,5 @@
 RSpec.describe "Making a search request" do
-  let(:search_service) { instance_double(DiscoveryEngine::Search, call: result_set) }
+  let(:search_service) { instance_double(DiscoveryEngine::Search, result_set:) }
   let(:result_set) { ResultSet.new(results:, total: 42, start: 21) }
   let(:results) { [Result.new(content_id: "123"), Result.new(content_id: "456")] }
 
@@ -25,7 +25,9 @@
     it "passes any query parameters to the search service in the expected format" do
       get "/search.json?q=garden+centres&start=11&count=22"
 
-      expect(search_service).to have_received(:call).with("garden centres", start: 11, count: 22)
+      expect(DiscoveryEngine::Search).to have_received(:new).with(
+        hash_including(q: "garden centres", start: "11", count: "22"),
+      )
     end
   end
 end

spec/services/discovery_engine/search_spec.rb

@@ -1,5 +1,5 @@
 RSpec.describe DiscoveryEngine::Search do
-  subject(:search) { described_class.new(client:) }
+  subject(:search) { described_class.new(query_params, client:) }
 
   let(:client) { double("SearchService::Client", search: search_return_value) }
 
@@ -25,9 +25,11 @@
     end
   end
 
-  describe "#call" do
+  describe "#result_set" do
+    subject!(:result_set) { search.result_set }
+
     context "when the search is successful" do
-      subject!(:result_set) { search.call("garden centres") }
+      let(:query_params) { { q: "garden centres" } }
 
       let(:search_return_value) { double(response: search_response) }
       let(:search_response) { double(total_size: 42, results:) }
@@ -58,7 +60,7 @@
       end
 
       context "when start and count are specified" do
-        subject!(:result_set) { search.call("garden centres", start: 11, count: 22) }
+        let(:query_params) { { q: "garden centres", start: "11", count: "22" } }
 
         it "calls the client with the expected parameters" do
           expect(client).to have_received(:search).with(
@@ -77,7 +79,7 @@
 
       context "when searching for a query that has a single best bets defined" do
         # see test section in YAML config
-        subject!(:result_set) { search.call("i want to test a single best bet") }
+        let(:query_params) { { q: "i want to test a single best bet" } }
 
         let(:expected_boost_specs) do
           super() + [{
@@ -99,7 +101,7 @@
 
       context "when searching for a query that has multiple best bets defined" do
         # see test section in YAML config
-        subject!(:result_set) { search.call("i want to test multiple best bets") }
+        let(:query_params) { { q: "i want to test multiple best bets" } }
 
         let(:expected_boost_specs) do
           super() + [{