From c666d8d8b91d8fed902613c1125e40e74f9ce61a Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Wed, 6 Dec 2023 15:55:52 +0000 Subject: [PATCH] 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 --- app/controllers/searches_controller.rb | 10 +---- app/services/discovery_engine/search.rb | 44 ++++++++++++------- config/brakeman.ignore | 29 ++++++++++++ spec/requests/search_request_spec.rb | 6 ++- spec/services/discovery_engine/search_spec.rb | 14 +++--- 5 files changed, 71 insertions(+), 32 deletions(-) create mode 100644 config/brakeman.ignore diff --git a/app/controllers/searches_controller.rb b/app/controllers/searches_controller.rb index d5f4f21..59126a0 100644 --- a/app/controllers/searches_controller.rb +++ b/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 diff --git a/app/services/discovery_engine/search.rb b/app/services/discovery_engine/search.rb index e750e71..e877fb8 100644 --- a/app/services/discovery_engine/search.rb +++ b/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 diff --git a/config/brakeman.ignore b/config/brakeman.ignore new file mode 100644 index 0000000..3e30cdf --- /dev/null +++ b/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" +} diff --git a/spec/requests/search_request_spec.rb b/spec/requests/search_request_spec.rb index 5bb30c7..e02afce 100644 --- a/spec/requests/search_request_spec.rb +++ b/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 diff --git a/spec/services/discovery_engine/search_spec.rb b/spec/services/discovery_engine/search_spec.rb index ff71577..1fa3bfa 100644 --- a/spec/services/discovery_engine/search_spec.rb +++ b/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() + [{