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.

Sign up for GitHub

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

SCC-4348: callnumber= and standard_number= param #420

Merged
merged 5 commits into from
Nov 25, 2024
Merged

SCC-4348: callnumber= and standard_number= param #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
68de1c2
Add support for callnumber= param
nonword Nov 18, 2024
7aa5e0f
Add support for standard_number=
nonword Nov 18, 2024
40f0837
Rm console.log
nonword Nov 18, 2024
98a13b0
Add additional adv search test
nonword Nov 19, 2024
d0d605a
Change qa log-level to info
nonword Nov 19, 2024
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
4 changes: 3 additions & 1 deletion lib/api-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
/**
* This class wraps request params to ease interpretting the query
**/
const { SEARCH_SCOPES } = require('./elasticsearch/config')

// Build regex pattern for matching a phrase fully enclosed in quotes (or smart quotes):
const QUOTE_CHARS = '"\u201C\u201D\u201E\u201F\u2033\u2036'
const IN_QUOTES_PATTERN = new RegExp(`^[${QUOTE_CHARS}][^${QUOTE_CHARS}]+[${QUOTE_CHARS}]$`)

class ApiRequest {
static ADVANCED_SEARCH_PARAMS = ['title', 'subject', 'contributor']
static ADVANCED_SEARCH_PARAMS = ['title', 'subject', 'contributor', 'callnumber', 'standard_number']
static IDENTIFIER_NUMBER_PARAMS = ['isbn', 'issn', 'lccn', 'oclc']

constructor (params) {
Expand All @@ -28,6 +29,7 @@ class ApiRequest {
advancedSearchParamsThatAreAlsoScopes () {
// Return search params that are also valid search_scope values
return ApiRequest.ADVANCED_SEARCH_PARAMS
.filter((key) => SEARCH_SCOPES[key])
.filter((key) => this.params[key])
}

Expand Down
46 changes: 35 additions & 11 deletions lib/elasticsearch/elastic-query-builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const ElasticQuery = require('./elastic-query')
const ApiRequest = require('../api-request')
const { escapeQuery, namedQuery, prefixMatch, termMatch, phraseMatch } = require('./utils')
const { regexEscape } = require('../util')

Expand Down Expand Up @@ -60,17 +61,6 @@ class ElasticQueryBuilder {
this.requireMultiMatch(SEARCH_SCOPES.all.fields)
}

// Coming from Adv Search? Add additional multi-match clauses for subject,
// contributor, title:
if (this.request.advancedSearchParamsThatAreAlsoScopes()) {
this.request
.advancedSearchParamsThatAreAlsoScopes()
.forEach((param) =>
// Require a match on subject, contributor, or title fields:
this.requireMultiMatch(SEARCH_SCOPES[param].fields, this.request.params[param])
)
}

if (this.request.hasSearch()) {
// Apply common boosting:
this.boostNyplOwned()
Expand All @@ -90,6 +80,9 @@ class ElasticQueryBuilder {
}
}

// Look for query params coming from Adv Search:
this.applyAdvancedSearchParams()

// Lastly, if any identifier-number params are present (lccn=, isbn=, etc),
// add those clauses:
if (this.request.hasIdentifierNumberParam()) {
Expand Down Expand Up @@ -464,6 +457,37 @@ class ElasticQueryBuilder {
this.query.addMust(should.length === 1 ? should[0] : { bool: { should } })
}

/**
* Handle use of subject=, contributor=, title=, & callnumber= Adv Search
* params.
* Note that the RC Adv Search page may also apply filters, which are
* handled by `applyFilters`.
**/
applyAdvancedSearchParams () {
// We're specifically interested in params that match supported search-
// scopes because we can build a whole ES query just using the existing
// logic for that search scope:
if (this.request.advancedSearchParamsThatAreAlsoScopes()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the advanced search params that are not also scopes? is that just filters?

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is older work, but I think this method wants a different name. It's not the search_scopeness of the params we're interested in, but rather that they're valid single value query params (as opposed to a compound q=spaghetti&search_scope=title format). the method itself isn't actually verifying that they are also search scopes, just that they are in the approved array of advanced search params.

Copy link
Member Author

Choose a reason for hiding this comment

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

The word "advanced" in "advanced search params" is just a reminder that this is for supporting params used in RC Adv Search, but actually there are many params besides q= that are not filters. For example isbn=, issn=. In the future there could be more. The fact that these params are also valid search_scope values is actually important because we're leveraging the existing search-scope logic to build the sub-query. We very much want each of the adv search params to individually work the same as the namesake search scopes do, so we do want to use the same logic. The method thus does verify that they are also search-scopes (recent update here). In the future, there may be other Adv Search params (e.g. 'ISBN'), which would be included in an "approved array of advanced search params" but wold not have a namesake search-scope. To support that, we would have to add custom handler code to add the extra clauses for that new adv-search param. The fact that all non-filter adv search params happen to have equivalent search-scopes right now is a convenience, but not one we assume will remain, hence the awkward function name.

this.request
.advancedSearchParamsThatAreAlsoScopes()
.forEach((advSearchParam) => {
const advSearchValue = this.request.params[advSearchParam]
// Build a new ApiRequest object for this search-scope:
const request = ApiRequest.fromParams({
Copy link
Contributor

Choose a reason for hiding this comment

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

is this pattern being used elsewhere? something about it makes me uneasy...

Copy link
Member Author

Choose a reason for hiding this comment

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

Because ApiRequest feels like something that should only ever wrap a real user-initiated API request? It's a light-weight model representing a user query. The Adv Search page essentially allows you to find the intersection of multiple queries, so I think it's fine, but can you say more about what is unsettling? Conceptually we do want the same query to be generated from q=foo&search_scope=title as is generated from title=foo, so the two routes need to ultimately share some code. Do you see a clearer way?

q: advSearchValue,
search_scope: advSearchParam
})

// Build the ES query for the search-scope and value:
const builder = ElasticQueryBuilder.forApiRequest(request)
const subquery = builder.query.toJson()

// Add the query to the greater ES query's must clauses:
this.query.addMust(subquery)
})
}
}

/**
* Examine request for user-filters. When found, add them to query.
*/
Expand Down
2 changes: 2 additions & 0 deletions lib/resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ const parseSearchParams = function (params) {
filters: { type: 'hash', fields: FILTER_CONFIG },
items_size: { type: 'int', default: 100, range: [0, 200] },
items_from: { type: 'int', default: 0 },
callnumber: { type: 'string' },
standard_number: { type: 'string' },
contributor: { type: 'string' },
title: { type: 'string' },
subject: { type: 'string' },
Expand Down
19 changes: 0 additions & 19 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,6 @@ exports.parseParam = function (val, spec) {
return val
}

exports.arrayIntersection = (a1, a2) => {
return a1.filter(function (n) {
return a2.indexOf(n) !== -1
})
}

/**
* Get array of key-value pairs for object
*
Expand All @@ -193,19 +187,6 @@ exports.objectEntries = (obj) => {
.map((key) => [key, obj[key]])
}

exports.gatherParams = function (req, acceptedParams) {
// If specific params configured, pass those to handler
// otherwise just pass `value` param (i.e. keyword search)
acceptedParams = (typeof acceptedParams === 'undefined') ? ['page', 'per_page', 'value', 'q', 'filters', 'contributor', 'subject', 'title', 'isbn', 'issn', 'lccn', 'oclc', 'merge_checkin_card_items', 'include_item_aggregations'] : acceptedParams

const params = {}
acceptedParams.forEach((k) => {
params[k] = req.query[k]
})
if (req.query.q) params.value = req.query.q
return params
}

/*
* Expects array of strings, numbers
*/
Expand Down
40 changes: 7 additions & 33 deletions routes/resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const gatherParams = require('../lib/util').gatherParams

const VER = '0.1'

module.exports = function (app) {
Expand All @@ -11,29 +9,6 @@ module.exports = function (app) {
next()
})

const standardParams = ['page',
'per_page',
'q',
'filters',
'expandContext',
'ext',
'field',
'sort',
'sort_direction',
'search_scope',
'all_items',
'items_size',
'items_from',
'contributor',
'title',
'subject',
'isbn',
'issn',
'lccn',
'oclc',
'merge_checkin_card_items',
'include_item_aggregations']

const respond = (res, _resp, params) => {
let contentType = 'application/ld+json'
if (params.ext === 'ntriples') contentType = 'text/plain'
Expand Down Expand Up @@ -65,23 +40,23 @@ module.exports = function (app) {
}

app.get(`/api/v${VER}/discovery/resources$`, function (req, res) {
const params = gatherParams(req, standardParams)
const params = req.query

return app.resources.search(params, { baseUrl: app.baseUrl }, req)
.then((resp) => respond(res, resp, params))
.catch((error) => handleError(res, error, params))
})

app.get(`/api/v${VER}/discovery/resources/aggregations`, function (req, res) {
const params = gatherParams(req, standardParams)
const params = req.query

return app.resources.aggregations(params, { baseUrl: app.baseUrl })
.then((resp) => respond(res, resp, params))
.catch((error) => handleError(res, error, params))
})

app.get(`/api/v${VER}/discovery/resources/aggregation/:field`, function (req, res) {
const params = Object.assign({}, gatherParams(req, standardParams), req.params)
const params = req.query

return app.resources.aggregation(params, { baseUrl: app.baseUrl })
.then((resp) => respond(res, resp, params))
Expand All @@ -95,11 +70,11 @@ module.exports = function (app) {
* /api/v${VER}/request/deliveryLocationsByBarcode?barcodes[]=12345&barcodes[]=45678&barcodes=[]=78910
*/
app.get(`/api/v${VER}/request/deliveryLocationsByBarcode`, function (req, res) {
const params = gatherParams(req, ['barcodes', 'patronId'])
const params = req.query

const handler = app.resources.deliveryLocationsByBarcode

return handler(params, { baseUrl: app.baseUrl })
return handler(req.query.params, { baseUrl: app.baseUrl })
.then((resp) => respond(res, resp, params))
.catch((error) => handleError(res, error, params))
})
Expand All @@ -125,11 +100,10 @@ module.exports = function (app) {
* e.g. discovery/resources/b1234
*/
app.get(`/api/v${VER}/discovery/resources/:uri.:ext?`, function (req, res) {
const gatheredParams = gatherParams(req, ['uri', 'items_size', 'items_from', 'merge_checkin_card_items', 'include_item_aggregations', 'all_items'])
const params = Object.assign({}, req.query, { uri: req.params.uri })

if (Number.isInteger(parseInt(gatheredParams.items_size))) params.items_size = gatheredParams.items_size
if (Number.isInteger(parseInt(gatheredParams.items_from))) params.items_from = gatheredParams.items_from
if (Number.isInteger(parseInt(req.query.items_size))) params.items_size = req.query.items_size
if (Number.isInteger(parseInt(req.query.items_from))) params.items_from = req.query.items_from

let handler = app.resources.findByUri

Expand Down
Loading
Loading