Support per_page parameter #939
Conversation
Why these changes are being introduced: As we begin to experiment with reranking results from multiple APIs, it will be useful to configure TIMDEX to return a flexible number of results. Relevant ticket(s): - [TIMX-593](https://mitlibraries.atlassian.net/browse/TIMX-593) How this addresses that need: This updates the OpenSearch model to calculate query size based on the presence of a per_page param. The model continues to fall back on the SIZE and MAX_PAGE constants if no param is provided. Similarly, the QueryType class is updated to provide a per_page query param. We here we set the default value to 20 as a secondary layer of defense against bad inputs. Side effects of this change: New VCR cassette to cover the GraphQL query behavior. Normally I prefer stubs/mocks, but since this is a single additional test in a suite that solely uses VCR, it felt appropriate to continue that practice.
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
I think it would be best to remove the OpenSearch reference in the public graphql description and consider renaming MAX_PAGE to MAX_SIZE to clarify the intent.
app/models/opensearch.rb
Outdated
| calculate_size = if @params && @params[:per_page] | ||
| per_page = @params[:per_page].to_i | ||
| per_page = SIZE if per_page <= 0 | ||
| [per_page, MAX_PAGE].min |
There was a problem hiding this comment.
I wonder what our initial intent of MAX_PAGE was. It doesn't seem to be in use at this time.
I think removing it and setting a clearer variable such as MAX_SIZE might be clearer later.
app/graphql/types/query_type.rb
Outdated
| argument :from, String, required: false, default_value: '0', | ||
| description: 'Search result number to begin with (the first result is 0)' | ||
| argument :per_page, Integer, required: false, default_value: 20, | ||
| description: 'Number of results per page (OpenSearch size). Defaults to 20.' |
There was a problem hiding this comment.
I'm not sure if noting in the public API that this is an OpenSearch size equivalent is helpful or confusing...
| # allow overriding the OpenSearch `size` via params (per_page), capped by MAX_PAGE | ||
| calculate_size = if @params && @params[:per_page] | ||
| per_page = @params[:per_page].to_i | ||
| per_page = SIZE if per_page <= 0 |
There was a problem hiding this comment.
Ruby treating strings it doesn't know how to convert to an integer as zero is either brilliant or super weird. In this case it works well so I guess I should appreciate it.
Why these changes are being introduced:
As we begin to experiment with reranking results
from multiple APIs, it will be useful to configure TIMDEX to return a flexible number of results.
Relevant ticket(s):
How this addresses that need:
This updates the OpenSearch model to calculate
query size based on the presence of a per_page
param. The model continues to fall back on the
SIZE and MAX_PAGE constants if no param is
provided.
Similarly, the QueryType class is updated to
provide a per_page query param. We here we set
the default value to 20 as a secondary layer of
defense against bad inputs.
Side effects of this change:
New VCR cassette to cover the GraphQL query
behavior. Normally I prefer stubs/mocks, but since this is a single additional test in a suite that
solely uses VCR, it felt appropriate to continue
that practice.
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