Add targeted field search to GraphQL V2 #517
Conversation
Why these changes are being introduced: The data model specifies that certain fields must be single-field searchable. Relevant ticket(s): https://mitlibraries.atlassian.net/browse/RDI-103 How this addresses that need: This makes the following fields searchable in GraphQL: * citation * contributors * funding_information * identifiers * location * subjects * title This also renames the facet fields to avoid conflicts (see side effects for more information on this). Side effects of this change: _This introduces a breaking change_. There is some overlap between the fields above and facets. In order to accommodate both searching and faceting, '_facet' has been added to all of the facets in the QueryType class, and the corresponding change has been made in the Opensearch model. This means that anyone using the previous syntax for facets will see their queries fail. We will want to do more analysis of how the API is used before deploying this to prod. If we see usage of faceting in prod, we might consider renaming the search fields instead of the facets.
There was a problem hiding this comment.
I've got two questions about the lines I noted above, but in general I think this is solid.
My more general concern is about the timeline between merging this and getting it into production. It sounds like we still need to start collecting data about the use of these fields to confirm this approach, so it may not be a quick process? I worry about doing dependency updates - although maybe we can create a v1 branch or something from prior to this merging, and use that in the meantime.
If the two questions above are no issue, and the timeline seems doable, then I'm fine with this merging as-is.
app/graphql/types/query_type.rb
Outdated
| @@ -122,7 +137,7 @@ def construct_query(searchterm, facets) | |||
| query[:language] = facets[:languages] | |||
| query[:literary_form] = facets[:literary_form] | |||
| query[:source] = facets[:source] if facets[:source] != 'All' | |||
| query[:subject] = facets[:subjects] | |||
| query[:subjects_facet] = facets[:subjects] | |||
There was a problem hiding this comment.
I may have missed this discussion, but it looks like this change is being made to the v1 side of the flipflop? It doesn't look like this is matched by anything in the search model, so I'm not sure if I'm missing something.
There was a problem hiding this comment.
Yeah I think you are correct. As this should still be using ElasticSearch on the v1 side of the if/else this should remain the old name. I think.
There was a problem hiding this comment.
I've made this change in the commit I just added to the branch.
|
We should add a checkbox to the pr template in timdex something like "changes GraphQL schema" as a reminder that we need to update the schema we store in timdex-ui. Similar to the "requires database migrations" and "dep updates". I'll open a Jira ticket as it's not part of this work, but I wanted to share it here as a reminder to us all that now that timdex-ui stores the schema, we need to remember to refresh it when we make changes. |
- Changes a use of `subjects_facet` in the V1 branch back to `subjects` - Adds handling for `collection_facet` in the query types to match its presence in the opensearch model.
|
Okay @JPrevost - I've added a commit that addresses the questions I asked yesterday. Nothing else seems to be impacted, which makes me nervous that I'm not doing something right. The tests all passed before the change, and they all pass after the change. |
There was a problem hiding this comment.
@matt-bernhardt can you give the last commit I just added a look.
Why these changes are being introduced:
The data model specifies that certain fields must be
single-field searchable.
Relevant ticket(s):
https://mitlibraries.atlassian.net/browse/RDI-103
How this addresses that need:
This makes the following fields searchable in GraphQL:
This also renames the facet fields to avoid conflicts (see
side effects for more information on this).
Side effects of this change:
This introduces a breaking change. There is some overlap
between the fields above and facets. In order to accommodate both
searching and faceting, '_facet' has been added to all of the
facets in the QueryType class, and the corresponding change has
been made in the Opensearch model.
This means that anyone using the previous syntax for facets will
see their queries fail. We will want to do more analysis of how
the API is used before deploying this to prod. If we see usage
of faceting in prod, we might consider renaming the search fields
instead of the facets.
Requires Database Migrations?
NO
Includes new or updated dependencies?
NO