Allows searching the fulltext field when requested#933
Conversation
Why are these changes being introduced: * We are introducing a fulltext field in our OpenSearch index to allow searching the full text of documents. This change allows users to include the fulltext field in their search if they want. The default is false to support existing behavior (i.e. this is opt-in). Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-588 How does this address that need: * Moves fields to search into its own method so that it can conditionally include the fulltext field based on a parameter passed to the search method. * Updates GraphQL query_type to accept a fulltext parameter (defaulting to false) and pass that to the OpenSearch search method.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
This looks good to me - I've been able to use this argument in the sandbox and see a different number of records come back (more records if I set fulltext to true, which is what I'd expect). I have one question about the conditions under which the internal argument might vary in the way we're permitting, but that's only so that I make sure I understand things, not with a concern that this is the wrong change.
Thanks!
|
|
||
| # 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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
3 participants
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO
Changes GraphQL schema?
YES