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.

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 4346/non roman 2 #423

Merged
merged 9 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
28 changes: 14 additions & 14 deletions lib/elasticsearch/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,20 @@ const SEARCH_SCOPES = {
}

const FILTER_CONFIG = {
recordType: { operator: 'match', field: 'recordTypeId', repeatable: true },
owner: { operator: 'match', field: 'items.owner_packed', repeatable: true, path: 'items' },
subjectLiteral: { operator: 'match', field: 'subjectLiteral_exploded', repeatable: true },
holdingLocation: { operator: 'match', field: 'items.holdingLocation_packed', repeatable: true, path: 'items' },
buildingLocation: { operator: 'match', field: 'buildingLocationIds', repeatable: true },
language: { operator: 'match', field: 'language_packed', repeatable: true },
materialType: { operator: 'match', field: 'materialType_packed', repeatable: true },
mediaType: { operator: 'match', field: 'mediaType_packed', repeatable: true },
carrierType: { operator: 'match', field: 'carrierType_packed', repeatable: true },
publisher: { operator: 'match', field: 'publisherLiteral.raw', repeatable: true },
contributorLiteral: { operator: 'match', field: 'contributorLiteral.raw', repeatable: true },
creatorLiteral: { operator: 'match', field: 'creatorLiteral.raw', repeatable: true },
issuance: { operator: 'match', field: 'issuance_packed', repeatable: true },
createdYear: { operator: 'match', field: 'createdYear', repeatable: true },
recordType: { operator: 'match', field: ['recordTypeId'], repeatable: true },
owner: { operator: 'match', field: ['items.owner_packed'], repeatable: true, path: 'items' },
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't need to happen as part of this, but because this enables matching on multiple fields, you could change these _packed configs to match explicitly on each entity subfield. For example, above could use field ['items.owner.id', 'items.owner.label'], since that's how the clause ultimately manifests, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yess I like that. I knew there was something there because their structure was so similar.

subjectLiteral: { operator: 'match', field: ['subjectLiteral_exploded'], repeatable: true },
holdingLocation: { operator: 'match', field: ['items.holdingLocation_packed'], repeatable: true, path: 'items' },
buildingLocation: { operator: 'match', field: ['buildingLocationIds'], repeatable: true },
language: { operator: 'match', field: ['language_packed'], repeatable: true },
materialType: { operator: 'match', field: ['materialType_packed'], repeatable: true },
mediaType: { operator: 'match', field: ['mediaType_packed'], repeatable: true },
carrierType: { operator: 'match', field: ['carrierType_packed'], repeatable: true },
publisher: { operator: 'match', field: ['publisherLiteral.raw'], repeatable: true },
contributorLiteral: { operator: 'match', field: ['contributorLiteral.raw'], repeatable: true },
charmingduchess marked this conversation as resolved.
Show resolved Hide resolved
creatorLiteral: { operator: 'match', field: ['creatorLiteral.raw'], repeatable: true },
issuance: { operator: 'match', field: ['issuance_packed'], repeatable: true },
createdYear: { operator: 'match', field: ['createdYear'], repeatable: true },
dateAfter: {
operator: 'custom',
type: 'int'
Expand Down
84 changes: 52 additions & 32 deletions lib/elasticsearch/elastic-query-builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,57 @@ class ElasticQueryBuilder {
}
}

buildPackedFieldClause (value, field) {
charmingduchess marked this conversation as resolved.
Show resolved Hide resolved
charmingduchess marked this conversation as resolved.
Show resolved Hide resolved
// Figure out the base property (e.g. 'owner')
const baseField = field.replace(/_packed$/, '')
// Allow supplied val to match against either id or value:
return {
bool: {
should: [
{ term: { [`${baseField}.id`]: value } },
{ term: { [`${baseField}.label`]: value } }
]
}
}
}

buildMultiFieldClause (value, fields) {
return {
bool:
{ should: fields.map(field => ({ term: { [field]: value } })) }
}
}

// This builds a filter cause from the value:
buildClause (value, field) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems too general a name for the specific thing it's doing? Could it be:

buildFilterClause (value, fieldConfig)

const filterMatchesOnMoreThanOneField = field.length > 1
if (filterMatchesOnMoreThanOneField) {
return this.buildMultiFieldClause(value, field)
}
field = field[0]
const valueIsNotPackedValue = value.indexOf('||') < 0
const isPackedField = field.match(/_packed$/)
if (isPackedField && valueIsNotPackedValue) {
return this.buildPackedFieldClause(value, field)
charmingduchess marked this conversation as resolved.
Show resolved Hide resolved
} else return { term: { [field]: value } }
}

buildSimpleMatchFilters (simpleMatchFilters) {
return simpleMatchFilters.map((prop) => {
const config = FILTER_CONFIG[prop]
let value = this.request.params.filters[prop]

// If multiple values given, let's join them with 'should', causing it to operate as a boolean OR
// Note: using 'must' here makes it a boolean AND
const booleanOperator = 'should'

if (Array.isArray(value) && value.length === 1) value = value.shift()
const clause = (Array.isArray(value)) ? { bool: { [booleanOperator]: value.map((value) => this.buildClause(config.field, value)) } } : this.buildClause(config.field, value)

return { path: config.path, clause }
})
}

/**
* Examine request for user-filters. When found, add them to query.
*/
Expand All @@ -505,38 +556,7 @@ class ElasticQueryBuilder {
const simpleMatchFilters = Object.keys(this.request.params.filters)
.filter((k) => FILTER_CONFIG[k].operator === 'match')

filterClausesWithPaths = filterClausesWithPaths.concat(simpleMatchFilters.map((prop) => {
const config = FILTER_CONFIG[prop]

let value = this.request.params.filters[prop]

// This builds a filter cause from the value:
const buildClause = (value) => {
// If filtering on a packed field and value isn't a packed value:
if (config.operator === 'match' && value.indexOf('||') < 0 && config.field.match(/_packed$/)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed these references to config.operator === 'match' because we are passing in an array that is already filtered on that condition.

// Figure out the base property (e.g. 'owner')
const baseField = config.field.replace(/_packed$/, '')
// Allow supplied val to match against either id or value:
return {
bool: {
should: [
{ term: { [`${baseField}.id`]: value } },
{ term: { [`${baseField}.label`]: value } }
]
}
}
} else if (config.operator === 'match') return { term: { [config.field]: value } }
}

// If multiple values given, let's join them with 'should', causing it to operate as a boolean OR
// Note: using 'must' here makes it a boolean AND
const booleanOperator = 'should'
// If only one value given, don't wrap it in a useless bool:
if (Array.isArray(value) && value.length === 1) value = value.shift()
const clause = (Array.isArray(value)) ? { bool: { [booleanOperator]: value.map(buildClause) } } : buildClause(value)

return { path: config.path, clause }
}))
filterClausesWithPaths = filterClausesWithPaths.concat(this.buildSimpleMatchFilters(simpleMatchFilters))

// Gather root (not nested) filters:
let filterClauses = filterClausesWithPaths
Expand Down
Empty file.
117 changes: 117 additions & 0 deletions test/elastic-query-builder.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,123 @@ const ElasticQueryBuilder = require('../lib/elasticsearch/elastic-query-builder'
const ApiRequest = require('../lib/api-request')

describe('ElasticQueryBuilder', () => {
describe('buildClause', () => {
it('can handle multiple fields', () => {
expect(ElasticQueryBuilder.prototype.buildClause('value', ['field', 'parallelField']))
.to.deep.equal({
bool:
{
should: [
{ term: { field: 'value' } },
{ term: { parallelField: 'value' } }]
}
})
})
it('can handle packed fields', () => {
expect(ElasticQueryBuilder.prototype.buildClause('not packed value', ['field_packed']))
.to.deep.equal({
bool: {
should: [
{ term: { 'field.id': 'not packed value' } },
{ term: { 'field.label': 'not packed value' } }
]
}
})
})
it('can handle the simple case', () => {
expect(ElasticQueryBuilder.prototype.buildClause('value', ['field']))
.to.deep.equal({ term: { field: 'value' } })
})
})
describe('buildSimpleMatchFilters', () => {
const mockQueryBuilderFactory = (request) => ({
request,
buildMultiFieldClause: ElasticQueryBuilder.prototype.buildMultiFieldClause,
buildSimpleMatchFilters: ElasticQueryBuilder.prototype.buildSimpleMatchFilters,
buildClause: ElasticQueryBuilder.prototype.buildClause,
buildPackedFieldClause: ElasticQueryBuilder.prototype.buildPackedFieldClause
Copy link
Member

Choose a reason for hiding this comment

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

Can be removd

})
it('can handle (multiple) single value, single match field filters, as arrays', () => {
const request = new ApiRequest({ filters: { buildingLocation: ['toast'], subjectLiteral: ['spaghetti'] } })
const mockQueryBuilder = mockQueryBuilderFactory(request)
const simpleMatchFilters = mockQueryBuilder.buildSimpleMatchFilters(['buildingLocation', 'subjectLiteral'])
expect(simpleMatchFilters).to.deep.equal([
{
path: undefined,
clause: { term: { buildingLocationIds: 'toast' } }
},
{
path: undefined,
clause: { term: { subjectLiteral_exploded: 'spaghetti' } }
}
])
})
it('can handle (multiple) single value, single match field filters, strings', () => {
const request = new ApiRequest({ filters: { buildingLocation: 'toast', subjectLiteral: 'spaghetti' } })
const mockQueryBuilder = mockQueryBuilderFactory(request)
const simpleMatchFilters = mockQueryBuilder.buildSimpleMatchFilters(['buildingLocation', 'subjectLiteral'])
expect(simpleMatchFilters).to.deep.equal([
{
path: undefined,
clause: { term: { buildingLocationIds: 'toast' } }
},
{
path: undefined,
clause: { term: { subjectLiteral_exploded: 'spaghetti' } }
}
])
})
it('can handle multiple values', () => {
const request = new ApiRequest({ filters: { subjectLiteral: ['spaghetti', 'meatballs'] } })
const mockQueryBuilder = mockQueryBuilderFactory(request)
const simpleMatchFilters = mockQueryBuilder.buildSimpleMatchFilters(['subjectLiteral'])
expect(simpleMatchFilters).to.deep.equal([
{
path: undefined,
clause: {
bool: {
should: [
{ term: { subjectLiteral_exploded: 'spaghetti' } },
{ term: { subjectLiteral_exploded: 'meatballs' } }
]
}
}
}
])
})
it('can handle packed values', () => {
const request = new ApiRequest({ filters: { language: ['spanish', 'finnish'] } })
const mockQueryBuilder = mockQueryBuilderFactory(request)
const simpleMatchFilters = mockQueryBuilder.buildSimpleMatchFilters(['language'])
expect(simpleMatchFilters).to.deep.equal([
{
path: undefined,
clause: {
bool: {
should: [
{
bool: {
should: [
{ term: { 'language.id': 'spanish' } },
{ term: { 'language.label': 'spanish' } }
]
}
},
{
bool: {
should: [
{ term: { 'language.id': 'finnish' } },
{ term: { 'language.label': 'finnish' } }
]
}
}
]
}
}
}
])
})
})
describe('search_scope all', () => {
it('generates an "all" query', () => {
const request = new ApiRequest({ q: 'toast' })
Expand Down
Loading