From 2ff7709d22f8f9f5348c0a724640423605c2bc67 Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Wed, 7 Feb 2024 17:51:06 +0100 Subject: [PATCH 1/8] search: Omit some fields from object to index --- src/common/helpers/search.js | 17 +++++++++-------- src/server/routes/entity/entity.tsx | 6 +++--- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/common/helpers/search.js b/src/common/helpers/search.js index 9ef5b446f..1a26d65f1 100644 --- a/src/common/helpers/search.js +++ b/src/common/helpers/search.js @@ -64,6 +64,7 @@ async function _fetchEntityModelsForESResults(orm, results) { .fetch({withRelated: ['areaType']}); const areaJSON = area.toJSON(); + const areaJSON = area.toJSON({omitPivot: true}); const areaParents = await area.parents(); areaJSON.defaultAlias = { name: areaJSON.name @@ -78,7 +79,7 @@ async function _fetchEntityModelsForESResults(orm, results) { const editor = await Editor.forge({id: entityStub.bbid}) .fetch(); - const editorJSON = editor.toJSON(); + const editorJSON = editor.toJSON({omitPivot: true}); editorJSON.defaultAlias = { name: editorJSON.name }; @@ -90,7 +91,7 @@ async function _fetchEntityModelsForESResults(orm, results) { const collection = await UserCollection.forge({id: entityStub.bbid}) .fetch(); - const collectionJSON = collection.toJSON(); + const collectionJSON = collection.toJSON({omitPivot: true}); collectionJSON.defaultAlias = { name: collectionJSON.name }; @@ -103,7 +104,7 @@ async function _fetchEntityModelsForESResults(orm, results) { const entity = await model.forge({bbid: entityStub.bbid}) .fetch({require: false, withRelated: ['defaultAlias.language', 'disambiguation', 'aliasSet.aliases', 'identifierSet.identifiers', 'relationshipSet.relationships.source', 'relationshipSet.relationships.target', 'relationshipSet.relationships.type', 'annotation']}); - const entityJSON = entity?.toJSON(); + const entityJSON = entity?.toJSON({omitPivot: true}); if (entityJSON && entityJSON.relationshipSet) { entityJSON.relationshipSet.relationships = await Promise.all(entityJSON.relationshipSet.relationships.map(async (rel) => { rel.source = await commonUtils.getEntity(orm, rel.source.bbid, rel.source.type); @@ -384,7 +385,7 @@ export async function generateIndex(orm) { {model: EditionGroup, relations: []}, {model: Publisher, relations: ['area']}, {model: Series, relations: ['seriesOrderingType']}, - {model: Work, relations: ['workType', 'relationshipSet.relationships.type']} + {model: Work, relations: ['relationshipSet.relationships.type']} ]; // Update the indexed entries for each entity type @@ -427,7 +428,7 @@ export async function generateIndex(orm) { }); // Index all the entities for (const entityList of entityLists) { - const listArray = entityList.toJSON(); + const listArray = entityList.toJSON({omitPivot: true}); listIndexes.push(_processEntityListForBulk(listArray)); } await Promise.all(listIndexes); @@ -435,7 +436,7 @@ export async function generateIndex(orm) { const areaCollection = await Area.forge() .fetchAll(); - const areas = areaCollection.toJSON(); + const areas = areaCollection.toJSON({omitPivot: true}); /** To index names, we use aliasSet.aliases.name and bbid, which Areas don't have. * We massage the area to return a similar format as BB entities @@ -455,7 +456,7 @@ export async function generateIndex(orm) { // no bots .where('type_id', 1) .fetchAll(); - const editors = editorCollection.toJSON(); + const editors = editorCollection.toJSON({omitPivot: true}); /** To index names, we use aliasSet.aliases.name and bbid, which Editors don't have. * We massage the editor to return a similar format as BB entities @@ -473,7 +474,7 @@ export async function generateIndex(orm) { const userCollections = await UserCollection.forge().where({public: true}) .fetchAll(); - const userCollectionsJSON = userCollections.toJSON(); + const userCollectionsJSON = userCollections.toJSON({omitPivot: true}); /** To index names, we use aliasSet.aliases.name and bbid, which UserCollections don't have. * We massage the editor to return a similar format as BB entities diff --git a/src/server/routes/entity/entity.tsx b/src/server/routes/entity/entity.tsx index 5839cac14..6b46762cf 100644 --- a/src/server/routes/entity/entity.tsx +++ b/src/server/routes/entity/entity.tsx @@ -478,7 +478,7 @@ export function handleDelete( editorJSON.id, body.note ); - return savedMainEntity.toJSON(); + return savedMainEntity.toJSON({omitPivot: true}); }); return handler.sendPromiseResult(res, entityDeletePromise, search.deleteEntity); @@ -1047,7 +1047,7 @@ export async function indexAutoCreatedEditionGroup(orm, newEdition, transacting) transacting, withRelated: 'aliasSet.aliases' }); - await search.indexEntity(editionGroup.toJSON()); + await search.indexEntity(editionGroup.toJSON({omitPivot: true})); } catch (err) { log.error('Could not reindex edition group after edition creation:', err); @@ -1165,7 +1165,7 @@ export async function processSingleEntity(formBody, JSONEntity, reqSession, await indexAutoCreatedEditionGroup(orm, savedMainEntity, transacting); } - const entityJSON = savedMainEntity.toJSON(); + const entityJSON = savedMainEntity.toJSON({omitPivot: true}); if (entityJSON && entityJSON.relationshipSet) { entityJSON.relationshipSet.relationships = await Promise.all(entityJSON.relationshipSet.relationships.map(async (rel) => { try { From d14102081d3af7c775fa75c836a633a5ab033e94 Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Wed, 7 Feb 2024 17:52:01 +0100 Subject: [PATCH 2/8] search: catch more error scenarios while indexing --- src/common/helpers/search.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/common/helpers/search.js b/src/common/helpers/search.js index 1a26d65f1..777be92be 100644 --- a/src/common/helpers/search.js +++ b/src/common/helpers/search.js @@ -63,7 +63,6 @@ async function _fetchEntityModelsForESResults(orm, results) { const area = await Area.forge({gid: entityStub.bbid}) .fetch({withRelated: ['areaType']}); - const areaJSON = area.toJSON(); const areaJSON = area.toJSON({omitPivot: true}); const areaParents = await area.parents(); areaJSON.defaultAlias = { @@ -161,14 +160,14 @@ export async function _bulkIndexEntities(entities) { // eslint-disable-next-line no-await-in-loop const response = await _client.bulk({ body: bulkOperations - }); + }).catch(error => { log.error('error bulk indexing entities for search:', error); }); /* * In case of failed index operations, the promise won't be rejected; * instead, we have to inspect the response and respond to any failures * individually. */ - if (response.errors === true) { + if (response?.errors === true) { entitiesToIndex = response.items.reduce((accumulator, item) => { // We currently only handle queue overrun if (item.index.status === httpStatus.TOO_MANY_REQUESTS) { @@ -268,7 +267,7 @@ export function deleteEntity(entity) { } export function refreshIndex() { - return _client.indices.refresh({index: _index}); + return _client.indices.refresh({index: _index}).catch(error => { log.error('error refreshing search index:', error); }); } export async function generateIndex(orm) { @@ -414,14 +413,14 @@ export async function generateIndex(orm) { const relationshipSet = workEntity.related('relationshipSet'); if (relationshipSet) { const authorWroteWorkRels = relationshipSet.related('relationships')?.filter(relationshipModel => relationshipModel.get('typeId') === 8); - const authorNames = authorWroteWorkRels.map(relationshipModel => { + const authorNames = []; + authorWroteWorkRels.forEach(relationshipModel => { // Search for the Author in the already fetched BookshelfJS Collection const source = authorsCollection.get(relationshipModel.get('sourceBbid')); - if (!source) { - // This shouldn't happen, but just in case - return null; + const name = source?.related('defaultAlias')?.get('name'); + if (name) { + authorNames.push(name); } - return source.toJSON().defaultAlias.name; }); workEntity.set('authors', authorNames); } From ac497d3dae6b186976e00ad1c2fcb18ea7404abf Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Fri, 9 Feb 2024 12:42:22 +0100 Subject: [PATCH 3/8] chore(search): Make search tools a Typescript file --- src/common/helpers/{search.js => search.ts} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename src/common/helpers/{search.js => search.ts} (99%) diff --git a/src/common/helpers/search.js b/src/common/helpers/search.ts similarity index 99% rename from src/common/helpers/search.js rename to src/common/helpers/search.ts index 777be92be..e053d3b9a 100644 --- a/src/common/helpers/search.js +++ b/src/common/helpers/search.ts @@ -17,7 +17,7 @@ */ /* eslint-disable camelcase */ -import * as commonUtils from '../../common/helpers/utils'; +import * as commonUtils from './utils'; import {camelCase, isString, snakeCase, upperFirst} from 'lodash'; import ElasticSearch from '@elastic/elasticsearch'; @@ -495,7 +495,7 @@ export async function generateIndex(orm) { export async function checkIfExists(orm, name, type) { const {bookshelf} = orm; - const bbids = await new Promise((resolve, reject) => { + const bbids:string[] = await new Promise((resolve, reject) => { bookshelf.transaction(async (transacting) => { try { const result = await orm.func.alias.getBBIDsWithMatchingAlias( From d451b0a05d1d1ce22d945457ebaa15d1682dda83 Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Fri, 9 Feb 2024 13:26:39 +0100 Subject: [PATCH 4/8] chore(search): Index only the required fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have been storing a ton of information in the search index that we just won't ever use such as set ids and internal props from the ORM Model (_pivot…). Instead, let's pass the ORM models along and create a new utility to strip the dcument to index down to what we actually need. --- src/common/helpers/search.ts | 104 +++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 36 deletions(-) diff --git a/src/common/helpers/search.ts b/src/common/helpers/search.ts index e053d3b9a..813e11c60 100644 --- a/src/common/helpers/search.ts +++ b/src/common/helpers/search.ts @@ -21,6 +21,7 @@ import * as commonUtils from './utils'; import {camelCase, isString, snakeCase, upperFirst} from 'lodash'; import ElasticSearch from '@elastic/elasticsearch'; +import type {EntityTypeString} from 'bookbrainz-data/lib/types/entity'; import httpStatus from 'http-status'; import log from 'log'; @@ -48,6 +49,36 @@ function sanitizeEntityType(type) { return snakeCase(type); } +const commonProperties = ['bbid', 'name', 'type', 'disambiguation']; + +/* We don't currently want to index the entire Model in ElasticSearch, + which contains a lot of fields we don't use as well as some internal props (_pivot props) + This utility function prepares the Model into a minimal object that will be indexed +*/ +export function getDocumentToIndex(entity:any, entityType:EntityTypeString) { + const additionalProperties = []; + switch (entityType) { + case 'Work': + additionalProperties.push('authors'); + break; + default: + break; + } + let aliases = entity.related('aliasSet')?.related('aliases')?.toJSON({ignorePivot: true, visible: 'name'}); + if (!aliases) { + // Some models don't have the same aliasSet structure, i.e. Collection, Editor, Area, … + const name = entity.get('name'); + aliases = {name}; + } + const identifiers = entity.related('identifierSet')?.related('identifiers')?.toJSON({ignorePivot: true, visible: 'value'}); + + return { + ...entity.toJSON({ignorePivot: true, visible: commonProperties.concat(additionalProperties)}), + aliases, + identifiers + }; +} + async function _fetchEntityModelsForESResults(orm, results) { const {Area, Editor, UserCollection} = orm; @@ -223,7 +254,7 @@ export async function autocomplete(orm, query, type, size = 42) { else { queryBody = { match: { - 'aliasSet.aliases.name.autocomplete': { + 'aliases.name.autocomplete': { minimum_should_match: '80%', query } @@ -248,12 +279,14 @@ export async function autocomplete(orm, query, type, size = 42) { // eslint-disable-next-line consistent-return export function indexEntity(entity) { + const entityType = entity.get('type'); + const document = getDocumentToIndex(entity, entityType); if (entity) { return _client.index({ - body: entity, - id: entity.bbid, + body: document, + id: entity.get('bbid'), index: _index, - type: snakeCase(entity.type) + type: snakeCase(entityType) }).catch(error => { log.error('error indexing entity for search:', error); }); } } @@ -276,7 +309,7 @@ export async function generateIndex(orm) { mappings: { _default_: { properties: { - 'aliasSet.aliases': { + aliases: { properties: { name: { fields: { @@ -291,8 +324,7 @@ export async function generateIndex(orm) { }, type: 'text' } - }, - type: 'object' + } }, authors: { analyzer: 'trigrams', @@ -342,6 +374,8 @@ export async function generateIndex(orm) { } } } + }, + 'index.mapping.ignore_malformed': true } }; @@ -359,7 +393,6 @@ export async function generateIndex(orm) { const baseRelations = [ 'annotation', - 'defaultAlias', 'aliasSet.aliases', 'identifierSet.identifiers' ]; @@ -400,6 +433,7 @@ export async function generateIndex(orm) { ); const entityLists = await Promise.all(behaviorPromise); /* eslint-disable @typescript-eslint/no-unused-vars */ + const entityFetchOrder:EntityTypeString[] = ['Author', 'Edition', 'EditionGroup', 'Publisher', 'Series', 'Work']; const [authorsCollection, editionCollection, editionGroupCollection, @@ -426,10 +460,12 @@ export async function generateIndex(orm) { } }); // Index all the entities - for (const entityList of entityLists) { - const listArray = entityList.toJSON({omitPivot: true}); + entityLists.forEach((entityList, idx) => { + const entityType:EntityTypeString = entityFetchOrder[idx]; + const listArray = entityList.map(entity => getDocumentToIndex(entity, entityType)); listIndexes.push(_processEntityListForBulk(listArray)); - } + }); + await Promise.all(listIndexes); const areaCollection = await Area.forge() @@ -439,13 +475,13 @@ export async function generateIndex(orm) { /** To index names, we use aliasSet.aliases.name and bbid, which Areas don't have. * We massage the area to return a similar format as BB entities - */ + /** To index names, we use aliases.name and bbid, which Areas don't have. + * We massage the area to return a similar format as BB entities + */ const processedAreas = areas.map((area) => new Object({ - aliasSet: { - aliases: [ - {name: area.name} - ] - }, + aliases: [ + {name: area.name} + ], bbid: area.gid, type: 'Area' })); @@ -457,15 +493,13 @@ export async function generateIndex(orm) { .fetchAll(); const editors = editorCollection.toJSON({omitPivot: true}); - /** To index names, we use aliasSet.aliases.name and bbid, which Editors don't have. - * We massage the editor to return a similar format as BB entities - */ + /** To index names, we use aliases.name and bbid, which Editors don't have. + * We massage the editor to return a similar format as BB entities + */ const processedEditors = editors.map((editor) => new Object({ - aliasSet: { - aliases: [ - {name: editor.name} - ] - }, + aliases: [ + {name: editor.name} + ], bbid: editor.id, type: 'Editor' })); @@ -475,15 +509,13 @@ export async function generateIndex(orm) { .fetchAll(); const userCollectionsJSON = userCollections.toJSON({omitPivot: true}); - /** To index names, we use aliasSet.aliases.name and bbid, which UserCollections don't have. - * We massage the editor to return a similar format as BB entities - */ + /** To index names, we use aliases.name and bbid, which UserCollections don't have. + * We massage the editor to return a similar format as BB entities + */ const processedCollections = userCollectionsJSON.map((collection) => new Object({ - aliasSet: { - aliases: [ - {name: collection.name} - ] - }, + aliases: [ + {name: collection.name} + ], bbid: collection.id, id: collection.id, type: 'Collection' @@ -535,10 +567,10 @@ export function searchByName(orm, name, type, size, from) { query: { multi_match: { fields: [ - 'aliasSet.aliases.name^3', - 'aliasSet.aliases.name.search', + 'aliases.name^3', + 'aliases.name.search', 'disambiguation', - 'identifierSet.identifiers.value' + 'identifiers.value' ], minimum_should_match: '80%', query: name, From 4767a89032e5935ad9303b91ede687ebc6cdba67 Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Fri, 9 Feb 2024 13:32:46 +0100 Subject: [PATCH 5/8] chore(search): Send entity models for indexing With the change from the previous commit (accepting an ORM model rather than JSON for search indexing), we need to rewrite accordingly the parts of the code that use the search indexing. Taking this opportunity to rewrite some code from promises to async/await syntax. --- src/common/helpers/search.ts | 3 - src/server/helpers/collectionRouteUtils.js | 20 +--- src/server/routes/editor.tsx | 33 +++--- src/server/routes/entity/entity.tsx | 33 +++--- .../routes/entity/process-unified-form.ts | 4 +- src/server/routes/register.js | 100 ++++++++---------- 6 files changed, 77 insertions(+), 116 deletions(-) diff --git a/src/common/helpers/search.ts b/src/common/helpers/search.ts index 813e11c60..57a416ced 100644 --- a/src/common/helpers/search.ts +++ b/src/common/helpers/search.ts @@ -373,7 +373,6 @@ export async function generateIndex(orm) { type: 'ngram' } } - } }, 'index.mapping.ignore_malformed': true } @@ -473,8 +472,6 @@ export async function generateIndex(orm) { const areas = areaCollection.toJSON({omitPivot: true}); - /** To index names, we use aliasSet.aliases.name and bbid, which Areas don't have. - * We massage the area to return a similar format as BB entities /** To index names, we use aliases.name and bbid, which Areas don't have. * We massage the area to return a similar format as BB entities */ diff --git a/src/server/helpers/collectionRouteUtils.js b/src/server/helpers/collectionRouteUtils.js index bd018365b..286c50c5a 100644 --- a/src/server/helpers/collectionRouteUtils.js +++ b/src/server/helpers/collectionRouteUtils.js @@ -16,11 +16,10 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ -import * as handler from '../helpers/handler'; import * as search from '../../common/helpers/search'; import {camelCase, differenceWith, isEqual, toLower, upperFirst} from 'lodash'; import {BadRequestError} from '../../common/helpers/error'; - +import log from 'log'; /** * A handler for create or edit actions on collections. @@ -89,22 +88,9 @@ export async function collectionCreateOrEditHandler(req, res, next) { // if it's new or the name is changed, // we need to update this collection in ElasticSearch index if (isNew || res.locals.collection.name !== newCollection.get('name')) { - const collectionPromiseForES = new Promise((resolve) => { - const collectionForES = { - aliasSet: { - aliases: [ - {name: newCollection.get('name')} - ] - }, - bbid: newCollection.get('id'), - id: newCollection.get('id'), - type: 'Collection' - }; - resolve(collectionForES); - }); - return handler.sendPromiseResult(res, collectionPromiseForES, search.indexEntity); + await search.indexEntity(newCollection); } - return res.send(newCollection.toJSON()); + return res.status(200).send(newCollection.toJSON()); } catch (err) { return next(err); diff --git a/src/server/routes/editor.tsx b/src/server/routes/editor.tsx index dd6aa8145..de20c06cd 100644 --- a/src/server/routes/editor.tsx +++ b/src/server/routes/editor.tsx @@ -20,7 +20,6 @@ import * as auth from '../helpers/auth'; import * as commonUtils from '../../common/helpers/utils'; import * as error from '../../common/helpers/error'; -import * as handler from '../helpers/handler'; import * as propHelpers from '../../client/helpers/props'; import * as search from '../../common/helpers/search'; import {AdminActionType, PrivilegeType} from '../../common/helpers/privileges-utils'; @@ -42,6 +41,7 @@ import _ from 'lodash'; import express from 'express'; import {getOrderedCollectionsForEditorPage} from '../helpers/collections'; import {getOrderedRevisionForEditorPage} from '../helpers/revisions'; +import log from 'log'; import target from '../templates/target'; @@ -122,8 +122,8 @@ function isCurrentUser(reqUserID, sessionUser) { return reqUserID === sessionUser.id; } -router.post('/edit/handler', auth.isAuthenticatedForHandler, (req, res) => { - async function runAsync() { +router.post('/edit/handler', auth.isAuthenticatedForHandler, async (req, res) => { + try { const {Editor} = req.app.locals.orm; if (!isCurrentUser(req.body.id, req.user)) { @@ -140,10 +140,10 @@ router.post('/edit/handler', auth.isAuthenticatedForHandler, (req, res) => { throw new error.NotFoundError('Editor not found', req); }); if (editor.get('areaId') === req.body.areaId && - editor.get('bio') === req.body.bio && - editor.get('name') === req.body.name && - editor.get('titleUnlockId') === req.body.title && - editor.get('genderId') === req.body.genderId + editor.get('bio') === req.body.bio && + editor.get('name') === req.body.name && + editor.get('titleUnlockId') === req.body.title && + editor.get('genderId') === req.body.genderId ) { throw new error.FormSubmissionError('No change to the profile'); } @@ -164,19 +164,14 @@ router.post('/edit/handler', auth.isAuthenticatedForHandler, (req, res) => { }); res.locals.user.name = req.body.name; - const editorJSON = modifiedEditor.toJSON(); - return { - aliasSet: { - aliases: [ - {name: editorJSON.name} - ] - }, - bbid: editorJSON.id, - type: 'Editor' - }; - } - handler.sendPromiseResult(res, runAsync(), search.indexEntity); + await search.indexEntity(modifiedEditor); + + return res.status(200).send(modifiedEditor.toJSON()); + } + catch (err) { + return error.sendErrorAsJSON(res, err); + } }); router.post('/privs/edit/handler', auth.isAuthenticatedForHandler, auth.isAuthorized(ADMIN), async (req, res, next) => { diff --git a/src/server/routes/entity/entity.tsx b/src/server/routes/entity/entity.tsx index 6b46762cf..3cbc39e80 100644 --- a/src/server/routes/entity/entity.tsx +++ b/src/server/routes/entity/entity.tsx @@ -1165,33 +1165,28 @@ export async function processSingleEntity(formBody, JSONEntity, reqSession, await indexAutoCreatedEditionGroup(orm, savedMainEntity, transacting); } - const entityJSON = savedMainEntity.toJSON({omitPivot: true}); - if (entityJSON && entityJSON.relationshipSet) { - entityJSON.relationshipSet.relationships = await Promise.all(entityJSON.relationshipSet.relationships.map(async (rel) => { + const entityRelationships = savedMainEntity.related('relationshipSet')?.related('relationships'); + if (savedMainEntity.get('type') === 'Work' && entityRelationships?.length) { + const authorsOfWork = await Promise.all(entityRelationships.toJSON().filter( + // "Author wrote Work" relationship + (relation) => relation.typeId === 8 + ).map(async (relationshipJSON) => { try { - rel.source = await commonUtils.getEntity(orm, rel.source.bbid, rel.source.type, {require: false, transacting}); - rel.target = await commonUtils.getEntity(orm, rel.target.bbid, rel.target.type, {require: false, transacting}); + const {source} = relationshipJSON; + const sourceEntity = await commonUtils.getEntity( + orm, source.bbid, source.type, {require: false, transacting} + ); + return sourceEntity.name; } catch (err) { log.error(err); } - return rel; + return null; })); // Attach a work's authors for search indexing - if (savedMainEntity.get('type') === 'Work') { - const authorsOfWork = entityJSON.relationshipSet.relationships - .filter( - // "Author wrote Work" relationship - (relation) => relation.typeId === 8 - ) - .map((relation) => { - const {source} = relation; - return source.name; - }); - entityJSON.authors = authorsOfWork; - } + savedMainEntity.set('authors', authorsOfWork.filter(Boolean)); } - return entityJSON; + return savedMainEntity; } catch (err) { log.error(err); diff --git a/src/server/routes/entity/process-unified-form.ts b/src/server/routes/entity/process-unified-form.ts index 7b3726904..9a1fdf581 100644 --- a/src/server/routes/entity/process-unified-form.ts +++ b/src/server/routes/entity/process-unified-form.ts @@ -2,6 +2,7 @@ import * as achievement from '../../helpers/achievement'; import type {Request as $Request, Response as $Response} from 'express'; import {FormSubmissionError, sendErrorAsJSON} from '../../../common/helpers/error'; +import {_bulkIndexEntities, getDocumentToIndex} from '../../../common/helpers/search'; import {authorToFormState, transformNewForm as authorTransform} from './author'; import {editionGroupToFormState, transformNewForm as editionGroupTransform} from './edition-group'; import {editionToFormState, transformNewForm as editionTransform} from './edition'; @@ -13,7 +14,6 @@ import type { EntityTypeString } from 'bookbrainz-data/lib/types/entity'; import _ from 'lodash'; -import {_bulkIndexEntities} from '../../../common/helpers/search'; import log from 'log'; import {processSingleEntity} from './entity'; @@ -206,7 +206,7 @@ export async function handleCreateMultipleEntities( } // log indexing error if any try { - _bulkIndexEntities(processedAchievements); + _bulkIndexEntities(processedEntities.map(en => getDocumentToIndex(en, en.get('type')))); } catch (err) { log.error(err); diff --git a/src/server/routes/register.js b/src/server/routes/register.js index 4a88d7ffa..109a75803 100644 --- a/src/server/routes/register.js +++ b/src/server/routes/register.js @@ -93,65 +93,53 @@ router.get('/details', middleware.loadGenders, (req, res) => { })); }); -router.post('/handler', (req, res) => { - const {Editor, EditorType} = req.app.locals.orm; +router.post('/handler', async (req, res) => { + try { + const {Editor, EditorType} = req.app.locals.orm; + + // Check whether the user is logged in - if so, redirect to profile page + if (req.user) { + return res.redirect(`/editor/${req.user.id}`); + } + + if (!req.session.mbProfile) { + return res.redirect('/auth'); + } + + // Fetch the default EditorType from the database + const editorType = await EditorType.forge({label: 'Editor'}) + .fetch({require: true}); + + // Create a new Editor and add to the database + + const editor = await new Editor({ + cachedMetabrainzName: req.session.mbProfile.sub, + genderId: req.body.gender, + metabrainzUserId: req.session.mbProfile.metabrainz_user_id, + name: req.body.displayName, + typeId: editorType.id + }) + .save(); - // Check whether the user is logged in - if so, redirect to profile page - if (req.user) { - return res.redirect(`/editor/${req.user.id}`); - } + req.session.mbProfile = null; - if (!req.session.mbProfile) { - return res.redirect('/auth'); + await search.indexEntity(editor); + return res.status(200).send(editor.toJSON()); + } + catch (err) { + if (_.isMatch(err, {constraint: 'editor_name_key'})) { + return error.sendErrorAsJSON(res, new error.FormSubmissionError( + 'That username already exists - please try using another,' + + ' or contact us to have your existing BookBrainz account' + + ' linked to a MusicBrainz account.' + )); + } + log.error(err); + + return error.sendErrorAsJSON(res, new error.FormSubmissionError( + 'Something went wrong when registering, please try again!' + )); } - - // Fetch the default EditorType from the database - const registerPromise = EditorType.forge({label: 'Editor'}) - .fetch({require: true}) - .then( - // Create a new Editor and add to the database - (editorType) => - new Editor({ - cachedMetabrainzName: req.session.mbProfile.sub, - genderId: req.body.gender, - metabrainzUserId: req.session.mbProfile.metabrainz_user_id, - name: req.body.displayName, - typeId: editorType.id - }) - .save() - ) - .then((editor) => { - req.session.mbProfile = null; - const editorJSON = editor.toJSON(); - - // in ES index, we're storing the editor as entity - const editorForES = {}; - editorForES.bbid = editorJSON.id; - editorForES.aliasSet = { - aliases: [ - {name: editorJSON.name} - ] - }; - editorForES.type = 'Editor'; - return editorForES; - }) - .catch((err) => { - log.debug(err); - - if (_.isMatch(err, {constraint: 'editor_name_key'})) { - throw new error.FormSubmissionError( - 'That username already exists - please try using another,' + - ' or contact us to have your existing BookBrainz account' + - ' linked to a MusicBrainz account.' - ); - } - - throw new error.FormSubmissionError( - 'Something went wrong when registering, please try again!' - ); - }); - - return handler.sendPromiseResult(res, registerPromise, search.indexEntity); }); export default router; From 9a12fdf300fbb92f3b513eb328e91205371e8dad Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Fri, 9 Feb 2024 19:50:15 +0100 Subject: [PATCH 6/8] search: Add defaultAlias back for indexing work author --- src/common/helpers/search.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/helpers/search.ts b/src/common/helpers/search.ts index 57a416ced..15ccf4d88 100644 --- a/src/common/helpers/search.ts +++ b/src/common/helpers/search.ts @@ -392,6 +392,7 @@ export async function generateIndex(orm) { const baseRelations = [ 'annotation', + 'defaultAlias', 'aliasSet.aliases', 'identifierSet.identifiers' ]; From 51af091fe5d1262ae6badf9ca4146037e4fa88fd Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Fri, 16 Feb 2024 12:29:26 +0100 Subject: [PATCH 7/8] feat(search): Finish adapting indexing mechanism Missed some places where we need to set attributes on the ORM models for editor, collections and other non-entity types. Cheeky async/await rewrite to clarify some of it. --- src/common/helpers/search.ts | 42 +++++++++++----------- src/server/helpers/collectionRouteUtils.js | 2 ++ src/server/routes/editor.tsx | 6 +++- src/server/routes/entity/entity.tsx | 19 +++++----- src/server/routes/register.js | 7 +++- 5 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/common/helpers/search.ts b/src/common/helpers/search.ts index 15ccf4d88..e1092311e 100644 --- a/src/common/helpers/search.ts +++ b/src/common/helpers/search.ts @@ -49,13 +49,14 @@ function sanitizeEntityType(type) { return snakeCase(type); } -const commonProperties = ['bbid', 'name', 'type', 'disambiguation']; +type IndexableEntities = EntityTypeString | 'Editor' | 'Collection' | 'Area'; +const commonProperties = ['bbid', 'id', 'name', 'type', 'disambiguation']; /* We don't currently want to index the entire Model in ElasticSearch, which contains a lot of fields we don't use as well as some internal props (_pivot props) This utility function prepares the Model into a minimal object that will be indexed */ -export function getDocumentToIndex(entity:any, entityType:EntityTypeString) { +export function getDocumentToIndex(entity:any, entityType: IndexableEntities) { const additionalProperties = []; switch (entityType) { case 'Work': @@ -75,7 +76,7 @@ export function getDocumentToIndex(entity:any, entityType:EntityTypeString) { return { ...entity.toJSON({ignorePivot: true, visible: commonProperties.concat(additionalProperties)}), aliases, - identifiers + identifiers: identifiers ?? null }; } @@ -91,7 +92,7 @@ async function _fetchEntityModelsForESResults(orm, results) { // Special cases first if (entityStub.type === 'Area') { - const area = await Area.forge({gid: entityStub.bbid}) + const area = await Area.forge({gid: entityStub.id}) .fetch({withRelated: ['areaType']}); const areaJSON = area.toJSON({omitPivot: true}); @@ -106,7 +107,7 @@ async function _fetchEntityModelsForESResults(orm, results) { return areaJSON; } if (entityStub.type === 'Editor') { - const editor = await Editor.forge({id: entityStub.bbid}) + const editor = await Editor.forge({id: entityStub.id}) .fetch(); const editorJSON = editor.toJSON({omitPivot: true}); @@ -114,11 +115,11 @@ async function _fetchEntityModelsForESResults(orm, results) { name: editorJSON.name }; editorJSON.type = 'Editor'; - editorJSON.bbid = entityStub.bbid; + editorJSON.id = entityStub.id; return editorJSON; } if (entityStub.type === 'Collection') { - const collection = await UserCollection.forge({id: entityStub.bbid}) + const collection = await UserCollection.forge({id: entityStub.id}) .fetch(); const collectionJSON = collection.toJSON({omitPivot: true}); @@ -126,7 +127,7 @@ async function _fetchEntityModelsForESResults(orm, results) { name: collectionJSON.name }; collectionJSON.type = 'Collection'; - collectionJSON.bbid = entityStub.bbid; + collectionJSON.id = entityStub.id; return collectionJSON; } // Regular entity @@ -176,7 +177,7 @@ export async function _bulkIndexEntities(entities) { const bulkOperations = entitiesToIndex.reduce((accumulator, entity) => { accumulator.push({ index: { - _id: entity.bbid, + _id: entity.bbid ?? entity.id, _index, _type: snakeCase(entity.type) } @@ -203,7 +204,7 @@ export async function _bulkIndexEntities(entities) { // We currently only handle queue overrun if (item.index.status === httpStatus.TOO_MANY_REQUESTS) { const failedEntity = entities.find( - (element) => element.bbid === item.index._id + (element) => (element.bbid ?? element.id) === item.index._id ); accumulator.push(failedEntity); @@ -284,7 +285,7 @@ export function indexEntity(entity) { if (entity) { return _client.index({ body: document, - id: entity.get('bbid'), + id: entity.get('bbid') || entity.get('id'), index: _index, type: snakeCase(entityType) }).catch(error => { log.error('error indexing entity for search:', error); }); @@ -293,7 +294,7 @@ export function indexEntity(entity) { export function deleteEntity(entity) { return _client.delete({ - id: entity.bbid, + id: entity.bbid ?? entity.id, index: _index, type: snakeCase(entity.type) }).catch(error => { log.error('error deleting entity from index:', error); }); @@ -473,14 +474,14 @@ export async function generateIndex(orm) { const areas = areaCollection.toJSON({omitPivot: true}); - /** To index names, we use aliases.name and bbid, which Areas don't have. + /** To index names, we use aliases.name and type, which Areas don't have. * We massage the area to return a similar format as BB entities */ - const processedAreas = areas.map((area) => new Object({ + const processedAreas = areas.map((area) => ({ aliases: [ {name: area.name} ], - bbid: area.gid, + id: area.gid, type: 'Area' })); await _processEntityListForBulk(processedAreas); @@ -491,14 +492,14 @@ export async function generateIndex(orm) { .fetchAll(); const editors = editorCollection.toJSON({omitPivot: true}); - /** To index names, we use aliases.name and bbid, which Editors don't have. + /** To index names, we use aliases.name and type, which Editors don't have. * We massage the editor to return a similar format as BB entities */ - const processedEditors = editors.map((editor) => new Object({ + const processedEditors = editors.map((editor) => ({ aliases: [ {name: editor.name} ], - bbid: editor.id, + id: editor.id, type: 'Editor' })); await _processEntityListForBulk(processedEditors); @@ -507,14 +508,13 @@ export async function generateIndex(orm) { .fetchAll(); const userCollectionsJSON = userCollections.toJSON({omitPivot: true}); - /** To index names, we use aliases.name and bbid, which UserCollections don't have. + /** To index names, we use aliases.name and type, which UserCollections don't have. * We massage the editor to return a similar format as BB entities */ - const processedCollections = userCollectionsJSON.map((collection) => new Object({ + const processedCollections = userCollectionsJSON.map((collection) => ({ aliases: [ {name: collection.name} ], - bbid: collection.id, id: collection.id, type: 'Collection' })); diff --git a/src/server/helpers/collectionRouteUtils.js b/src/server/helpers/collectionRouteUtils.js index 286c50c5a..24b8c534c 100644 --- a/src/server/helpers/collectionRouteUtils.js +++ b/src/server/helpers/collectionRouteUtils.js @@ -88,6 +88,8 @@ export async function collectionCreateOrEditHandler(req, res, next) { // if it's new or the name is changed, // we need to update this collection in ElasticSearch index if (isNew || res.locals.collection.name !== newCollection.get('name')) { + // Type needed for indexing + newCollection.set('type', 'Collection'); await search.indexEntity(newCollection); } return res.status(200).send(newCollection.toJSON()); diff --git a/src/server/routes/editor.tsx b/src/server/routes/editor.tsx index de20c06cd..e0ffbf335 100644 --- a/src/server/routes/editor.tsx +++ b/src/server/routes/editor.tsx @@ -165,9 +165,13 @@ router.post('/edit/handler', auth.isAuthenticatedForHandler, async (req, res) => res.locals.user.name = req.body.name; + const editorJSON = modifiedEditor.toJSON(); + + // Type needed for indexing + modifiedEditor.set('type', 'Editor'); await search.indexEntity(modifiedEditor); - return res.status(200).send(modifiedEditor.toJSON()); + return res.status(200).send(editorJSON); } catch (err) { return error.sendErrorAsJSON(res, err); diff --git a/src/server/routes/entity/entity.tsx b/src/server/routes/entity/entity.tsx index 3cbc39e80..21ebee34c 100644 --- a/src/server/routes/entity/entity.tsx +++ b/src/server/routes/entity/entity.tsx @@ -1194,7 +1194,7 @@ export async function processSingleEntity(formBody, JSONEntity, reqSession, } } -export function handleCreateOrEditEntity( +export async function handleCreateOrEditEntity( req: PassportRequest, res: $Response, entityType: EntityTypeString, @@ -1204,16 +1204,15 @@ export function handleCreateOrEditEntity( const {orm}: {orm?: any} = req.app.locals; const editorJSON = req.user; const {bookshelf} = orm; - const entityEditPromise = bookshelf.transaction((transacting) => + const savedEntityModel = await bookshelf.transaction((transacting) => processSingleEntity(req.body, res.locals.entity, req.session, entityType, orm, editorJSON, derivedProps, isMergeOperation, transacting)); - const achievementPromise = entityEditPromise.then( - (entityJSON) => processAchievement(orm, editorJSON.id, entityJSON) - ); - return handler.sendPromiseResult( - res, - achievementPromise, - search.indexEntity - ); + const entityJSON = savedEntityModel.toJSON(); + + await processAchievement(orm, editorJSON.id, entityJSON); + + await search.indexEntity(savedEntityModel); + + return res.status(200).send(entityJSON); } type AliasEditorT = { diff --git a/src/server/routes/register.js b/src/server/routes/register.js index 109a75803..56936a3eb 100644 --- a/src/server/routes/register.js +++ b/src/server/routes/register.js @@ -123,8 +123,13 @@ router.post('/handler', async (req, res) => { req.session.mbProfile = null; + const editorJSON = editor.toJSON(); + + // Type needed for indexing + editor.set('type', 'Editor'); await search.indexEntity(editor); - return res.status(200).send(editor.toJSON()); + + return res.status(200).send(editorJSON); } catch (err) { if (_.isMatch(err, {constraint: 'editor_name_key'})) { From a3360ff4cefae87e4841f62a7937b2f1eccb8451 Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Fri, 16 Feb 2024 12:53:40 +0100 Subject: [PATCH 8/8] chore(search): Allow for id field in results Not all entities have a BBID field now, some have "id" instead --- .../components/pages/parts/search-results.js | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/client/components/pages/parts/search-results.js b/src/client/components/pages/parts/search-results.js index c38291be0..da5d7c470 100644 --- a/src/client/components/pages/parts/search-results.js +++ b/src/client/components/pages/parts/search-results.js @@ -29,6 +29,12 @@ import {genEntityIconHTMLElement} from '../../../helpers/entity'; const {Alert, Badge, Button, ButtonGroup, Table} = bootstrap; +// Main entities have a BBID but some other indexed types +// have an ID field instead (collections, editors, areas) +function getId(entity) { + return entity.bbid ?? entity.id; +} + /** * Renders the document and displays the 'SearchResults' page. * @returns {ReactElement} a HTML document which displays the SearchResults. @@ -89,8 +95,8 @@ class SearchResults extends React.Component { // eslint-disable-next-line react/no-access-state-in-setstate const oldSelected = this.state.selected; let newSelected; - if (oldSelected.find(selected => selected.bbid === entity.bbid)) { - newSelected = oldSelected.filter(selected => selected.bbid !== entity.bbid); + if (oldSelected.find(selected => getId(selected) === getId(entity))) { + newSelected = oldSelected.filter(selected => getId(selected) !== getId(entity)); } else { newSelected = [...oldSelected, entity]; @@ -148,6 +154,7 @@ class SearchResults extends React.Component { if (!result) { return null; } + const id = getId(result); const name = result.defaultAlias ? result.defaultAlias.name : '(unnamed)'; @@ -160,19 +167,19 @@ class SearchResults extends React.Component { const disambiguation = result.disambiguation ? ({result.disambiguation.comment}) : ''; // No redirect link for Area entity results const link = result.type === 'Area' ? - `//musicbrainz.org/area/${result.bbid}` : - `/${_kebabCase(result.type)}/${result.bbid}`; + `//musicbrainz.org/area/${id}` : + `/${_kebabCase(result.type)}/${id}`; /* eslint-disable react/jsx-no-bind */ return ( - + { !this.props.condensed && { this.props.user ? selected.bbid === result.bbid)} + checked={this.state.selected.find(selected => getId(selected) === id)} className="checkboxes" type="checkbox" onChange={() => this.toggleRow(result)} @@ -212,7 +219,7 @@ class SearchResults extends React.Component { this.props.user ?
selected.bbid)} + bbids={this.state.selected.map(getId)} closeModalAndShowMessage={this.closeModalAndShowMessage} entityType={this.state.selected[0]?.type} handleCloseModal={this.onCloseModal}