Merge pull request #425 from NYPL/main

non roman author links bugfix deploy

lib/elasticsearch/config.js

@@ -1,3 +1,5 @@
+const util = require('../util')
+
 // Configure search scopes:
 const SEARCH_SCOPES = {
   all: {
@@ -74,20 +76,32 @@ 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.id', 'items.owner.label'], repeatable: true, path: 'items' },
+  subjectLiteral: {
+    transform: (val, logger) => {
+      if (typeof val === 'string') {
+        return util.removeTrailingPeriod(val, logger)
+      }
+      if (Array.isArray(val)) {
+        return val.map((val) => util.removeTrailingPeriod(val, logger))
+      }
+    },
+    operator: 'match',
+    field: ['subjectLiteral_exploded'],
+    repeatable: true
+  },
+  holdingLocation: { operator: 'match', field: ['items.holdingLocation.id', 'items.holdingLocation.label'], repeatable: true, path: 'items' },
+  buildingLocation: { operator: 'match', field: ['buildingLocationIds'], repeatable: true },
+  language: { operator: 'match', field: ['language.id', 'language.label'], repeatable: true },
+  materialType: { operator: 'match', field: ['materialType.id', 'materialType.label'], repeatable: true },
+  mediaType: { operator: 'match', field: ['mediaType.id', 'mediaType.label'], repeatable: true },
+  carrierType: { operator: 'match', field: ['carrierType.id', 'carrierType.label'], repeatable: true },
+  publisher: { operator: 'match', field: ['publisherLiteral.raw'], repeatable: true },
+  contributorLiteral: { operator: 'match', field: ['contributorLiteral.raw', 'parallelContributor.raw'], repeatable: true },
+  creatorLiteral: { operator: 'match', field: ['creatorLiteral.raw', 'parallelCreatorLiteral.raw'], repeatable: true },
+  issuance: { operator: 'match', field: ['issuance.id', 'issuance.label'], repeatable: true },
+  createdYear: { operator: 'match', field: ['createdYear'], repeatable: true },
   dateAfter: {
     operator: 'custom',
     type: 'int'

lib/elasticsearch/elastic-query-builder.js

@@ -488,6 +488,40 @@ class ElasticQueryBuilder {
     }
   }
 
+  buildMultiFieldClause (value, fields) {
+    return {
+      bool:
+        { should: fields.map(field => ({ term: { [field]: value } })) }
+    }
+  }
+
+  // This builds a filter cause from the value:
+  buildFilterClause (value, fieldsToMatchOn) {
+    const filterMatchesOnMoreThanOneField = fieldsToMatchOn.length > 1
+    if (filterMatchesOnMoreThanOneField) {
+      return this.buildMultiFieldClause(value, fieldsToMatchOn)
+    } else {
+      const field = fieldsToMatchOn[0]
+      return { term: { [field]: value } }
+    }
+  }
+
+  buildMatchOperatorFilterQueries (filtersWithMatchOperators) {
+    return filtersWithMatchOperators.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.buildFilterClause(value, config.field)) } } : this.buildFilterClause(value, config.field)
+
+      return { path: config.path, clause }
+    })
+  }
+
   /**
   * Examine request for user-filters. When found, add them to query.
   */
@@ -502,41 +536,10 @@ class ElasticQueryBuilder {
     )
 
     // Collect those filters that use a simple term match
-    const simpleMatchFilters = Object.keys(this.request.params.filters)
+    const filtersWithMatchOperators = 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$/)) {
-          // 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.buildMatchOperatorFilterQueries(filtersWithMatchOperators))
 
     // Gather root (not nested) filters:
     let filterClauses = filterClausesWithPaths

lib/util.js

@@ -1,6 +1,13 @@
 const logger = require('./logger')
 const { isItemNyplOwned } = require('./ownership_determination')
 
+exports.removeTrailingPeriod = (x, logger) => {
+  if (x.slice(-1) === '.') {
+    logger.debug('Removing terminal period', JSON.stringify(x, null, 4))
+    return x.slice(0, -1)
+  } else return x
+}
+
 exports.sortOnPropWithUndefinedLast = (property) => {
   return function (a, b) {
     // equal items sort equally
@@ -120,22 +127,8 @@ exports.parseParams = function (params, spec) {
 //   `fields`: Hash - When `type` is 'hash', this property provides field spec to validate internal fields against
 //   `repeatable`: Boolean - If true, array of values may be returned. Otherwise will select last. Default false
 exports.parseParam = function (val, spec) {
-  if (spec.fields &&
-    spec.fields.subjectLiteral &&
-    spec.fields.subjectLiteral.field === 'subjectLiteral_exploded' &&
-    val.subjectLiteral
-  ) {
-    if (typeof val.subjectLiteral === 'string' && val.subjectLiteral.slice(-1) === '.') {
-      val.subjectLiteral = val.subjectLiteral.slice(0, -1)
-      logger.debug('Removing terminal period', JSON.stringify(val, null, 4))
-    } else if (Array.isArray(val.subjectLiteral)) {
-      val.subjectLiteral.forEach((sub, i) => {
-        if (sub.slice(-1) === '.') {
-          val.subjectLiteral[i] = sub.slice(0, -1)
-          logger.debug('Removing terminal period', JSON.stringify(sub, null, 4))
-        }
-      })
-    }
+  if (spec.transform) {
+    val = spec.transform(val, logger)
   }
 
   // Unless it's marked repeatable, convert arrays of values to just last value:

test/elastic-query-builder.test.js

@@ -4,6 +4,111 @@ const ElasticQueryBuilder = require('../lib/elasticsearch/elastic-query-builder'
 const ApiRequest = require('../lib/api-request')
 
 describe('ElasticQueryBuilder', () => {
+  describe('buildFilterClause', () => {
+    it('can handle multiple fields', () => {
+      expect(ElasticQueryBuilder.prototype.buildFilterClause('value', ['field', 'parallelField']))
+        .to.deep.equal({
+          bool:
+          {
+            should: [
+              { term: { field: 'value' } },
+              { term: { parallelField: 'value' } }]
+          }
+        })
+    })
+    it('can handle the simple case', () => {
+      expect(ElasticQueryBuilder.prototype.buildFilterClause('value', ['field']))
+        .to.deep.equal({ term: { field: 'value' } })
+    })
+  })
+  describe('buildMatchOperatorFilterQueries', () => {
+    const mockQueryBuilderFactory = (request) => ({
+      request,
+      buildMultiFieldClause: ElasticQueryBuilder.prototype.buildMultiFieldClause,
+      buildMatchOperatorFilterQueries: ElasticQueryBuilder.prototype.buildMatchOperatorFilterQueries,
+      buildFilterClause: ElasticQueryBuilder.prototype.buildFilterClause
+    })
+    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.buildMatchOperatorFilterQueries(['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.buildMatchOperatorFilterQueries(['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.buildMatchOperatorFilterQueries(['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.buildMatchOperatorFilterQueries(['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' })

test/util.test.js

@@ -2,6 +2,7 @@ const { expect } = require('chai')
 const mangledEnumerationChronologyItems = require('./fixtures/mangled_enumerationChronology_items.json')
 
 const util = require('../lib/util')
+const { FILTER_CONFIG } = require('../lib/elasticsearch/config')
 
 describe('Util', function () {
   describe('sortOnPropWithUndefinedLast', () => {
@@ -64,19 +65,14 @@ describe('Util', function () {
         filters: {
           subjectLiteral: 'cats',
           contributorLiteral: ['Contrib 1', 'Contrib 2'],
-          date: '2012',
+          dateAfter: '2012',
           badNumeric: 'blah'
         }
       }
       const spec = {
         filters: {
           type: 'hash',
-          fields: {
-            subjectLiteral: { type: 'string' },
-            contributorLiteral: { type: 'string' },
-            date: { type: 'int' },
-            badNumeric: { type: 'int' }
-          }
+          fields: { ...FILTER_CONFIG, badNumeric: { type: 'int' } }
         }
       }
       const outgoing = util.parseParams(incoming, spec)
@@ -87,8 +83,8 @@ describe('Util', function () {
       expect(outgoing.filters.subjectLiteral).to.be.a('string')
       expect(outgoing.filters.subjectLiteral).to.equal('cats')
 
-      expect(outgoing.filters.date).to.be.a('number')
-      expect(outgoing.filters.date).to.equal(2012)
+      expect(outgoing.filters.dateAfter).to.be.a('number')
+      expect(outgoing.filters.dateAfter).to.equal(2012)
 
       expect(outgoing.filters.badNumeric).to.be.a('undefined')
     })
@@ -131,15 +127,7 @@ describe('Util', function () {
     }
 
     const spec = {
-      filters: {
-        type: 'hash',
-        fields: {
-          subjectLiteral: {
-            type: 'string',
-            field: 'subjectLiteral_exploded'
-          }
-        }
-      }
+      filters: { type: 'hash', fields: FILTER_CONFIG }
     }
 
     const outgoing = util.parseParams(incoming, spec)
@@ -154,16 +142,7 @@ describe('Util', function () {
     }
 
     const spec = {
-      filters: {
-        type: 'hash',
-        fields: {
-          subjectLiteral: {
-            type: 'string',
-            field: 'subjectLiteral_exploded',
-            repeatable: true
-          }
-        }
-      }
+      filters: { type: 'hash', fields: FILTER_CONFIG }
     }
 
     const outgoing = util.parseParams(incoming, spec)