Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow sorting of search results #142

Merged
merged 1 commit into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions app/services/discovery_engine/search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module DiscoveryEngine
class Search
DEFAULT_PAGE_SIZE = 10
DEFAULT_OFFSET = 0
DEFAULT_ORDER_BY = nil # not specifying an order_by means the results are ordered by relevance

def initialize(
query_params,
Expand All @@ -12,13 +13,7 @@ def initialize(
end

def result_set
response = client.search(
query:,
serving_config:,
page_size:,
offset:,
boost_spec:,
).response
response = client.search(discovery_engine_params).response

ResultSet.new(
results: response.results.map { Result.from_stored_document(_1.document.struct_data.to_h) },
Expand All @@ -31,6 +26,17 @@ def result_set

attr_reader :query_params, :client

def discovery_engine_params
{
query:,
serving_config:,
page_size:,
offset:,
order_by:,
boost_spec:,
}.compact
end

def query
query_params[:q].presence || ""
end
Expand All @@ -43,6 +49,25 @@ def offset
query_params[:start].presence&.to_i || DEFAULT_OFFSET
end

def order_by
case query_params[:order].presence
when "public_timestamp"
"public_timestamp"
when "-public_timestamp"
"public_timestamp desc"
when nil, "relevance", "popularity"
# "relevance" and "popularity" behave differently on the previous search-api, but we can
# treat them the same with Discovery Engine (as empty searches will default to a
# popularity-ish order anyway and we don't have an explicit popularity option available).
DEFAULT_ORDER_BY
else
# This helps us spot clients that are sending unexpected values and probably should continue
# to use the previoius search-api instead of this API.
Rails.logger.warn("Unexpected order_by value: #{query_params[:order].inspect}")
DEFAULT_ORDER_BY
end
end

def serving_config
Rails.configuration.discovery_engine_serving_config
end
Expand Down
30 changes: 30 additions & 0 deletions spec/services/discovery_engine/search_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,36 @@
end
end

context "when sorting by ascending public timestamp" do
let(:query_params) { { q: "garden centres", order: "public_timestamp" } }

it "calls the client with the expected parameters" do
expect(client).to have_received(:search).with(
hash_including(order_by: "public_timestamp"),
)
end
end

context "when sorting by descending public timestamp" do
let(:query_params) { { q: "garden centres", order: "-public_timestamp" } }

it "calls the client with the expected parameters" do
expect(client).to have_received(:search).with(
hash_including(order_by: "public_timestamp desc"),
)
end
end

context "when attempting to sort by an unexpected value" do
let(:query_params) { { q: "garden centres", order: "foobarbaz" } }

it "calls the client with the expected parameters" do
expect(client).to have_received(:search).with(
hash_not_including(:order_by),
)
end
end

context "when searching for a query that has a single best bets defined" do
# see test section in YAML config
let(:query_params) { { q: "i want to test a single best bet" } }
Expand Down