Skip to content
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
6 changes: 4 additions & 2 deletions app/graphql/types/query_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ def record_id(id:, index:)
argument :title, String, required: false, default_value: nil, description: 'Search by title'
argument :from, String, required: false, default_value: '0',
description: 'Search result number to begin with (the first result is 0)'
argument :fulltext, Boolean, required: false, default_value: false,
description: 'Include fulltext field in search? Defaults to false.'
argument :index, String, required: false, default_value: nil,
description: 'It is not recommended to provide an index value unless we have provided ' \
'you with one for your specific use case'
Expand Down Expand Up @@ -99,11 +101,11 @@ def record_id(id:, index:)
end

def search(searchterm:, citation:, contributors:, funding_information:, geodistance:, geobox:, identifiers:,
locations:, subjects:, title:, index:, source:, from:, boolean_type:, **filters)
locations:, subjects:, title:, index:, source:, from:, boolean_type:, fulltext:, **filters)
query = construct_query(searchterm, citation, contributors, funding_information, geodistance, geobox, identifiers,
locations, subjects, title, source, boolean_type, filters)

results = Opensearch.new.search(from, query, Timdex::OSClient, highlight_requested?, index)
results = Opensearch.new.search(from, query, Timdex::OSClient, highlight_requested?, index, fulltext)

response = {}
response[:hits] = results['hits']['total']['value']
Expand Down
22 changes: 18 additions & 4 deletions app/models/opensearch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,20 @@ class Opensearch
SIZE = 20
MAX_PAGE = 200

def search(from, params, client, highlight = false, index = nil)
def search(from, params, client, highlight = false, index = nil, fulltext = false)
@params = params
@highlight = highlight
@fulltext = fulltext?(fulltext)
index = default_index unless index.present?
client.search(index:,
body: build_query(from))
end

# Only treat fulltext as true if it is boolean true or the string 'true' (case insensitive)
def fulltext?(fulltext_param)
fulltext_param == true || fulltext_param.to_s.downcase == 'true'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a question only so I can understand - is it possible for this argument to vary in this way, the way we've built this? The query schema enforces fulltext to be a Boolean, and errors if a string is provided (at least as I poke it in the sandbox on the review app). I'm happy for the internals to be a bit more permissive in this way, but if there's a way that this variation might occur in the wild I'm missing it at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct in that if the GraphQL entry point is used this is enforced upstream. I don't necessarily trust future us to not use some other entry point that may not be enforced in the same way. In other words, I don't want to rely on GraphQL logic to ensure the OpenSearch logic is sound. Does that make sense?

end

def default_index
ENV.fetch('OPENSEARCH_INDEX', nil)
end
Expand Down Expand Up @@ -132,15 +138,23 @@ def minimum_should_match
end
end

# Fields to be searched in multi_match query. Adds 'fulltext' field if fulltext search is enabled.
def fields_to_search
fields = ['alternate_titles', 'call_numbers', 'citation', 'contents', 'contributors.value', 'dates.value',
'edition', 'funding_information.*', 'identifiers.value', 'languages', 'locations.value',
'notes.value', 'numbering', 'publication_information', 'subjects.value', 'summary', 'title']
fields << 'fulltext' if @fulltext

fields
end

def matches
m = []
if @params[:q].present?
m << {
multi_match: {
query: @params[:q].downcase,
fields: ['alternate_titles', 'call_numbers', 'citation', 'contents', 'contributors.value', 'dates.value',
'edition', 'funding_information.*', 'identifiers.value', 'languages', 'locations.value',
'notes.value', 'numbering', 'publication_information', 'subjects.value', 'summary', 'title'],
fields: fields_to_search,
minimum_should_match:
}
}
Expand Down
15 changes: 15 additions & 0 deletions test/models/opensearch_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,21 @@ class OpensearchTest < ActiveSupport::TestCase
end
end

test 'fulltext is included when requested' do
os = Opensearch.new
os.instance_variable_set(:@params, { q: 'this' })
os.instance_variable_set(:@fulltext, true)

assert(os.matches.to_json.include?('"fields":["alternate_titles","call_numbers","citation","contents","contributors.value","dates.value","edition","funding_information.*","identifiers.value","languages","locations.value","notes.value","numbering","publication_information","subjects.value","summary","title","fulltext"]'))
end

test 'fulltext is not included by default' do
os = Opensearch.new
os.instance_variable_set(:@params, { q: 'this' })

assert(os.matches.to_json.include?('"fields":["alternate_titles","call_numbers","citation","contents","contributors.value","dates.value","edition","funding_information.*","identifiers.value","languages","locations.value","notes.value","numbering","publication_information","subjects.value","summary","title"]'))
end

test 'searches a single field' do
VCR.use_cassette('opensearch single field') do
params = { title: 'spice it up' }
Expand Down